linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] partitions/ide: improve Host Protected Area handling
@ 2009-05-31 14:39 Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 1/4] partitions: warn about the partition exceeding device capacity Bartlomiej Zolnierkiewicz
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 14:39 UTC (permalink / raw)
  To: linux-ide
  Cc: Bartlomiej Zolnierkiewicz, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop


Hi,

Since from the perspective of most users of recent systems, disabling
Host Protected Area (HPA) can break vendor RAID formats, GPT partitions
and risks corrupting firmware or overwriting vendor system recovery tools
this patchset makes the IDE subsystem preserve HPA by default.

Unfortunately the original (kernels < 2.6.30) behavior (unconditionally
disabling HPA and using full disk capacity) was introduced at the time
when the main use of HPA was to make the drive look small enough for the
BIOS to allow the system to boot with large capacity drives.

Thus to allow the maximum compatibility with the existing setups (using
HPA and partitioned with HPA disabled) we automatically disable HPA if
any partitions overlapping HPA are detected.  Additionally HPA can also
be disabled using the "nohpa" module parameter (i.e. "ide_core.nohpa=0.0"
to disable HPA on /dev/hda).

I tested it with artificially created HPA (using 'hdparm -N p', kudos to
Mark Lord for that) and it worked as expected:

	hda: Host Protected Area detected.
		current capacity is 117210000 sectors (60011 MB)
		native  capacity is 117210240 sectors (60011 MB)
	hda: 117210000 sectors (60011 MB) w/7884KiB Cache, CHS=16383/255/63
	hda: cache flushes supported
	 hda: hda1 hda2 hda3 hda4 < hda5 hda6 >
	hda: p6 size 44869482 exceeds device capacity, enabling native capacity
	hda: detected capacity change from 60011520000 to 60011642880


Thanks to Robert Hancock, Frans Pop and Andries E. Brouwer for input/ideas on
previous (now obsoleted) HPA patches.

[ Robert, it seems possible to use ->set_capacity block device method also in
  libata to improving HPA handling and fix previously discussed compatibility
  issue. ]


patches:
 #01: warn about the partition exceeding device capacity in rescan_partions()

 #02: add ->set_capacity method to struct block_device_operations and use it
      in rescan_partitions() to enable native device capacity if any partition
      exceeding device capacity is detected

 #03: implement ->set_capacity method in ide-gd device driver to handle HPA

 #04: change the original kernel behavior and preserve HPA by default

 For easier testing the combined patch against 2.6.30-rc7 is available here:

	http://www.kernel.org/pub/linux/kernel/people/bart/hpa-2.6.30-rc7.patch


diffstat:
 Documentation/ide/ide.txt           |    2 +
 Documentation/kernel-parameters.txt |    7 +-----
 drivers/ide/ide-disk.c              |   22 ++++++++++++++++++
 drivers/ide/ide-gd.c                |   14 ++++++++++++
 drivers/ide/ide.c                   |   10 ++++++++
 fs/partitions/check.c               |   42 +++++++++++++++++++++++++++---------
 include/linux/blkdev.h              |    2 +
 include/linux/genhd.h               |    1 
 include/linux/ide.h                 |    6 +++--
 9 files changed, 88 insertions(+), 18 deletions(-)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/4] partitions: warn about the partition exceeding device capacity
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
@ 2009-05-31 14:39 ` Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 2/4] partitions: add ->set_capacity block device method Bartlomiej Zolnierkiewicz
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 14:39 UTC (permalink / raw)
  To: linux-ide
  Cc: Bartlomiej Zolnierkiewicz, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] partitions: warn about the partition exceeding device capacity

The current warning message says only about the kernel's action taken
without mentioning the underlying reason behind it.

Noticed-by: Robert Hancock <hancockrwd@gmail.com>
Cc: Frans Pop <elendil@planet.nl>
Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 fs/partitions/check.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: b/fs/partitions/check.c
===================================================================
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -574,7 +574,8 @@ int rescan_partitions(struct gendisk *di
 			 * creating invalid block devices
 			 */
 			printk(KERN_WARNING
-			       "%s: p%d size %llu limited to end of disk\n",
+			       "%s: p%d size %llu exceeds device capacity, "
+			       "limited to end of disk\n",
 			       disk->disk_name, p, (unsigned long long) size);
 			size = get_capacity(disk) - from;
 		}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/4] partitions: add ->set_capacity block device method
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 1/4] partitions: warn about the partition exceeding device capacity Bartlomiej Zolnierkiewicz
@ 2009-05-31 14:39 ` Bartlomiej Zolnierkiewicz
  2009-06-06  8:42   ` Al Viro
  2009-05-31 14:39 ` [PATCH 3/4] ide-gd: implement block device ->set_capacity method Bartlomiej Zolnierkiewicz
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 14:39 UTC (permalink / raw)
  To: linux-ide
  Cc: Bartlomiej Zolnierkiewicz, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] partitions: add ->set_capacity block device method

* Add ->set_capacity block device method and use it in rescan_partitions()
  to attempt enabling native capacity of the device upon detecting the
  partition which exceeds device capacity.

* Add GENHD_FL_NATIVE_CAPACITY flag to try limit attempts of enabling
  native capacity during partition scan.

Together with the consecutive patch implementing ->set_capacity method in
ide-gd device driver this allows automatic disabling of Host Protected Area
(HPA) if any partitions overlapping HPA are detected.

Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: Frans Pop <elendil@planet.nl>
Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 fs/partitions/check.c  |   43 ++++++++++++++++++++++++++++++++-----------
 include/linux/blkdev.h |    2 ++
 include/linux/genhd.h  |    1 +
 3 files changed, 35 insertions(+), 11 deletions(-)

Index: b/fs/partitions/check.c
===================================================================
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -556,28 +556,49 @@ int rescan_partitions(struct gendisk *di
 
 	/* add partitions */
 	for (p = 1; p < state->limit; p++) {
-		sector_t size = state->parts[p].size;
-		sector_t from = state->parts[p].from;
+		sector_t size, from;
+try_scan:
+		size = state->parts[p].size;
 		if (!size)
 			continue;
+
+		from = state->parts[p].from;
 		if (from >= get_capacity(disk)) {
 			printk(KERN_WARNING
 			       "%s: p%d ignored, start %llu is behind the end of the disk\n",
 			       disk->disk_name, p, (unsigned long long) from);
 			continue;
 		}
+
 		if (from + size > get_capacity(disk)) {
-			/*
-			 * we can not ignore partitions of broken tables
-			 * created by for example camera firmware, but we
-			 * limit them to the end of the disk to avoid
-			 * creating invalid block devices
-			 */
+			struct block_device_operations *bdops = disk->fops;
+			unsigned long long capacity;
+
 			printk(KERN_WARNING
-			       "%s: p%d size %llu exceeds device capacity, "
-			       "limited to end of disk\n",
+			       "%s: p%d size %llu exceeds device capacity, ",
 			       disk->disk_name, p, (unsigned long long) size);
-			size = get_capacity(disk) - from;
+
+			if (bdops->set_capacity &&
+			    (disk->flags & GENHD_FL_NATIVE_CAPACITY) == 0) {
+				printk(KERN_CONT "enabling native capacity\n");
+				capacity = bdops->set_capacity(disk, ~0ULL);
+				disk->flags |= GENHD_FL_NATIVE_CAPACITY;
+				if (capacity > get_capacity(disk)) {
+					set_capacity(disk, capacity);
+					check_disk_size_change(disk, bdev);
+					bdev->bd_invalidated = 0;
+				}
+				goto try_scan;
+			} else {
+				/*
+				 * we can not ignore partitions of broken tables
+				 * created by for example camera firmware, but
+				 * we limit them to the end of the disk to avoid
+				 * creating invalid block devices
+				 */
+				printk(KERN_CONT "limited to end of disk\n");
+				size = get_capacity(disk) - from;
+			}
 		}
 		part = add_partition(disk, p, from, size,
 				     state->parts[p].flags);
Index: b/include/linux/blkdev.h
===================================================================
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1221,6 +1221,8 @@ struct block_device_operations {
 	int (*direct_access) (struct block_device *, sector_t,
 						void **, unsigned long *);
 	int (*media_changed) (struct gendisk *);
+	unsigned long long (*set_capacity) (struct gendisk *,
+						unsigned long long);
 	int (*revalidate_disk) (struct gendisk *);
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
 	struct module *owner;
Index: b/include/linux/genhd.h
===================================================================
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -114,6 +114,7 @@ struct hd_struct {
 #define GENHD_FL_UP				16
 #define GENHD_FL_SUPPRESS_PARTITION_INFO	32
 #define GENHD_FL_EXT_DEVT			64 /* allow extended devt */
+#define GENHD_FL_NATIVE_CAPACITY		128
 
 #define BLK_SCSI_MAX_CMDS	(256)
 #define BLK_SCSI_CMD_PER_LONG	(BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/4] ide-gd: implement block device ->set_capacity method
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 1/4] partitions: warn about the partition exceeding device capacity Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 2/4] partitions: add ->set_capacity block device method Bartlomiej Zolnierkiewicz
@ 2009-05-31 14:39 ` Bartlomiej Zolnierkiewicz
  2009-06-01 21:32   ` Bartlomiej Zolnierkiewicz
  2009-05-31 14:39 ` [PATCH 4/4] ide: preserve Host Protected Area by default Bartlomiej Zolnierkiewicz
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 14:39 UTC (permalink / raw)
  To: linux-ide
  Cc: Bartlomiej Zolnierkiewicz, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide-gd: implement block device ->set_capacity method

* Use ->probed_capacity to store native device capacity for ATA disks.

* Add ->set_capacity method to struct ide_disk_ops.

* Implement disk device ->set_capacity method for ATA disks.

* Implement block device ->set_capacity method.

Together with the previous patch adding ->set_capacity block device
method this allows automatic disabling of Host Protected Area (HPA)
if any partitions overlapping HPA are detected.

Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: Frans Pop <elendil@planet.nl>
Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-disk.c |   19 ++++++++++++++++++-
 drivers/ide/ide-gd.c   |   14 ++++++++++++++
 include/linux/ide.h    |    4 ++--
 3 files changed, 34 insertions(+), 3 deletions(-)

Index: b/drivers/ide/ide-disk.c
===================================================================
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -323,6 +323,8 @@ static void idedisk_check_hpa(ide_drive_
 	if (set_max <= capacity)
 		return;
 
+	drive->probed_capacity = set_max;
+
 	printk(KERN_INFO "%s: Host Protected Area detected.\n"
 			 "\tcurrent capacity is %llu sectors (%llu MB)\n"
 			 "\tnative  capacity is %llu sectors (%llu MB)\n",
@@ -358,6 +360,8 @@ static int ide_disk_get_capacity(ide_dri
 		drive->capacity64 = drive->cyl * drive->head * drive->sect;
 	}
 
+	drive->probed_capacity = drive->capacity64;
+
 	if (lba) {
 		drive->dev_flags |= IDE_DFLAG_LBA;
 
@@ -376,7 +380,7 @@ static int ide_disk_get_capacity(ide_dri
 		       "%llu sectors (%llu MB)\n",
 		       drive->name, (unsigned long long)drive->capacity64,
 		       sectors_to_MB(drive->capacity64));
-		drive->capacity64 = 1ULL << 28;
+		drive->probed_capacity = drive->capacity64 = 1ULL << 28;
 	}
 
 	if ((drive->hwif->host_flags & IDE_HFLAG_NO_LBA48_DMA) &&
@@ -392,6 +396,18 @@ static int ide_disk_get_capacity(ide_dri
 	return 0;
 }
 
+static u64 ide_disk_set_capacity(ide_drive_t *drive, u64 capacity)
+{
+	u64 set = min(capacity, drive->probed_capacity);
+	int lba48 = ata_id_lba48_enabled(drive->id);
+
+	capacity = idedisk_set_max_address(drive, set, lba48);
+	if (capacity)
+		drive->capacity64 = capacity;
+
+	return drive->capacity64;
+}
+
 static void idedisk_prepare_flush(struct request_queue *q, struct request *rq)
 {
 	ide_drive_t *drive = q->queuedata;
@@ -740,6 +756,7 @@ static int ide_disk_set_doorlock(ide_dri
 
 const struct ide_disk_ops ide_ata_disk_ops = {
 	.check		= ide_disk_check,
+	.set_capacity	= ide_disk_set_capacity,
 	.get_capacity	= ide_disk_get_capacity,
 	.setup		= ide_disk_setup,
 	.flush		= ide_disk_flush,
Index: b/drivers/ide/ide-gd.c
===================================================================
--- a/drivers/ide/ide-gd.c
+++ b/drivers/ide/ide-gd.c
@@ -287,6 +287,19 @@ static int ide_gd_media_changed(struct g
 	return ret;
 }
 
+static unsigned long long ide_gd_set_capacity(struct gendisk *disk,
+					      unsigned long long capacity)
+{
+	struct ide_disk_obj *idkp = ide_drv_g(disk, ide_disk_obj);
+	ide_drive_t *drive = idkp->drive;
+	const struct ide_disk_ops *disk_ops = drive->disk_ops;
+
+	if (disk_ops->set_capacity)
+		return disk_ops->set_capacity(drive, capacity);
+
+	return drive->capacity64;
+}
+
 static int ide_gd_revalidate_disk(struct gendisk *disk)
 {
 	struct ide_disk_obj *idkp = ide_drv_g(disk, ide_disk_obj);
@@ -315,6 +328,7 @@ static struct block_device_operations id
 	.locked_ioctl		= ide_gd_ioctl,
 	.getgeo			= ide_gd_getgeo,
 	.media_changed		= ide_gd_media_changed,
+	.set_capacity		= ide_gd_set_capacity,
 	.revalidate_disk	= ide_gd_revalidate_disk
 };
 
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -381,6 +381,7 @@ struct ide_drive_s;
 struct ide_disk_ops {
 	int		(*check)(struct ide_drive_s *, const char *);
 	int		(*get_capacity)(struct ide_drive_s *);
+	u64		(*set_capacity)(struct ide_drive_s *, u64);
 	void		(*setup)(struct ide_drive_s *);
 	void		(*flush)(struct ide_drive_s *);
 	int		(*init_media)(struct ide_drive_s *, struct gendisk *);
@@ -552,8 +553,7 @@ struct ide_drive_s {
 	unsigned int	drive_data;	/* used by set_pio_mode/dev_select() */
 	unsigned int	failures;	/* current failure count */
 	unsigned int	max_failures;	/* maximum allowed failure count */
-	u64		probed_capacity;/* initial reported media capacity (ide-cd only currently) */
-
+	u64		probed_capacity;/* initial/native media capacity */
 	u64		capacity64;	/* total number of sectors */
 
 	int		lun;		/* logical unit */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 4/4] ide: preserve Host Protected Area by default
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
                   ` (2 preceding siblings ...)
  2009-05-31 14:39 ` [PATCH 3/4] ide-gd: implement block device ->set_capacity method Bartlomiej Zolnierkiewicz
@ 2009-05-31 14:39 ` Bartlomiej Zolnierkiewicz
  2009-06-01 21:33   ` Bartlomiej Zolnierkiewicz
  2009-05-31 15:24 ` [PATCH 0/4] partitions/ide: improve Host Protected Area handling Andries E. Brouwer
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 14:39 UTC (permalink / raw)
  To: linux-ide
  Cc: Bartlomiej Zolnierkiewicz, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: preserve Host Protected Area by default

>From the perspective of most users of recent systems, disabling Host
Protected Area (HPA) can break vendor RAID formats, GPT partitions and
risks corrupting firmware or overwriting vendor system recovery tools.

Unfortunately the original (kernels < 2.6.30) behavior (unconditionally
disabling HPA and using full disk capacity) was introduced at the time
when the main use of HPA was to make the drive look small enough for the
BIOS to allow the system to boot with large capacity drives.

Thus to allow the maximum compatibility with the existing setups (using
HPA and partitioned with HPA disabled) we automatically disable HPA if
any partitions overlapping HPA are detected.  Additionally HPA can also
be disabled using the "nohpa" module parameter (i.e. "ide_core.nohpa=0.0"
to disable HPA on /dev/hda).

While at it:
- remove stale "idebus=" entry from Documentation/kernel-parameters.txt

Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: Frans Pop <elendil@planet.nl>
Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
Cc: Al Viro <viro@zeniv.linux.org.uk>
[patch description was based on input from Alan Cox and Frans Pop]
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 Documentation/ide/ide.txt           |    2 ++
 Documentation/kernel-parameters.txt |    7 ++-----
 drivers/ide/ide-disk.c              |    3 +++
 drivers/ide/ide.c                   |   10 ++++++++++
 include/linux/ide.h                 |    2 ++
 5 files changed, 19 insertions(+), 5 deletions(-)

Index: b/Documentation/ide/ide.txt
===================================================================
--- a/Documentation/ide/ide.txt
+++ b/Documentation/ide/ide.txt
@@ -216,6 +216,8 @@ Other kernel parameters for ide_core are
 
 * "noflush=[interface_number.device_number]" to disable flush requests
 
+* "nohpa=[interface_number.device_number]" to disable Host Protected Area
+
 * "noprobe=[interface_number.device_number]" to skip probing
 
 * "nowerr=[interface_number.device_number]" to ignore the WRERR_STAT bit
Index: b/Documentation/kernel-parameters.txt
===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -872,11 +872,8 @@ and is between 256 and 4096 characters. 
 
 	ide-core.nodma=	[HW] (E)IDE subsystem
 			Format: =0.0 to prevent dma on hda, =0.1 hdb =1.0 hdc
-			.vlb_clock .pci_clock .noflush .noprobe .nowerr .cdrom
-			.chs .ignore_cable are additional options
-			See Documentation/ide/ide.txt.
-
-	idebus=		[HW] (E)IDE subsystem - VLB/PCI bus speed
+			.vlb_clock .pci_clock .noflush .nohpa .noprobe .nowerr
+			.cdrom .chs .ignore_cable are additional options
 			See Documentation/ide/ide.txt.
 
 	ide-pci-generic.all-generic-ide [HW] (E)IDE subsystem
Index: b/drivers/ide/ide-disk.c
===================================================================
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -332,6 +332,9 @@ static void idedisk_check_hpa(ide_drive_
 			 capacity, sectors_to_MB(capacity),
 			 set_max, sectors_to_MB(set_max));
 
+	if ((drive->dev_flags & IDE_DFLAG_NOHPA) == 0)
+		return;
+
 	set_max = idedisk_set_max_address(drive, set_max, lba48);
 
 	if (set_max) {
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -211,6 +211,11 @@ static unsigned int ide_noflush;
 module_param_call(noflush, ide_set_dev_param_mask, NULL, &ide_noflush, 0);
 MODULE_PARM_DESC(noflush, "disable flush requests for a device");
 
+static unsigned int ide_nohpa;
+
+module_param_call(nohpa, ide_set_dev_param_mask, NULL, &ide_nohpa, 0);
+MODULE_PARM_DESC(nohpa, "disable Host Protected Area for a device");
+
 static unsigned int ide_noprobe;
 
 module_param_call(noprobe, ide_set_dev_param_mask, NULL, &ide_noprobe, 0);
@@ -281,6 +286,11 @@ static void ide_dev_apply_params(ide_dri
 				 drive->name);
 		drive->dev_flags |= IDE_DFLAG_NOFLUSH;
 	}
+	if (ide_nohpa & (1 << i)) {
+		printk(KERN_INFO "ide: disabling Host Protected Area for %s\n",
+				 drive->name);
+		drive->dev_flags |= IDE_DFLAG_NOHPA;
+	}
 	if (ide_noprobe & (1 << i)) {
 		printk(KERN_INFO "ide: skipping probe for %s\n", drive->name);
 		drive->dev_flags |= IDE_DFLAG_NOPROBE;
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -459,6 +459,8 @@ enum {
 	IDE_DFLAG_NICE1			= (1 << 5),
 	/* device is physically present */
 	IDE_DFLAG_PRESENT		= (1 << 6),
+	/* disable Host Protected Area */
+	IDE_DFLAG_NOHPA			= (1 << 7),
 	/* id read from device (synthetic if not set) */
 	IDE_DFLAG_ID_READ		= (1 << 8),
 	IDE_DFLAG_NOPROBE		= (1 << 9),

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
                   ` (3 preceding siblings ...)
  2009-05-31 14:39 ` [PATCH 4/4] ide: preserve Host Protected Area by default Bartlomiej Zolnierkiewicz
@ 2009-05-31 15:24 ` Andries E. Brouwer
  2009-05-31 15:34   ` Bartlomiej Zolnierkiewicz
  2009-05-31 16:36 ` Alan Cox
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andries E. Brouwer @ 2009-05-31 15:24 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

On Sun, May 31, 2009 at 04:39:11PM +0200, Bartlomiej Zolnierkiewicz wrote:

[HPA stuff deleted]

Hi Bartlomiej,

Apart from HPA one also has DCO, which functions rather similarly.
(I have not checked whether the current kernel knows anything about DCO.)
Have you thought about DCO?
Probably the kernel should by default not touch any such setting.
Maybe it should be reported?
A user space utility might change this setting - thus a disk can change size.

Andries


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 15:24 ` [PATCH 0/4] partitions/ide: improve Host Protected Area handling Andries E. Brouwer
@ 2009-05-31 15:34   ` Bartlomiej Zolnierkiewicz
  2009-06-01 13:02     ` Greg Freemyer
  0 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-05-31 15:34 UTC (permalink / raw)
  To: Andries E. Brouwer
  Cc: linux-ide, linux-kernel, Robert Hancock, Al Viro, Frans Pop

On Sunday 31 May 2009 17:24:03 Andries E. Brouwer wrote:
> On Sun, May 31, 2009 at 04:39:11PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> [HPA stuff deleted]
> 
> Hi Bartlomiej,
> 
> Apart from HPA one also has DCO, which functions rather similarly.
> (I have not checked whether the current kernel knows anything about DCO.)
> Have you thought about DCO?
> Probably the kernel should by default not touch any such setting.
> Maybe it should be reported?
> A user space utility might change this setting - thus a disk can change size.

We leave DCO handling entirely to user-space currently.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
                   ` (4 preceding siblings ...)
  2009-05-31 15:24 ` [PATCH 0/4] partitions/ide: improve Host Protected Area handling Andries E. Brouwer
@ 2009-05-31 16:36 ` Alan Cox
  2009-05-31 20:04 ` Frans Pop
  2009-06-01 12:59 ` Greg Freemyer
  7 siblings, 0 replies; 20+ messages in thread
From: Alan Cox @ 2009-05-31 16:36 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Bartlomiej Zolnierkiewicz, Andries E. Brouwer,
	linux-kernel, Robert Hancock, Al Viro, Frans Pop

Excellent stuff.

Emphatically-Acked-by: Alan Cox <alan@linux.intel.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
                   ` (5 preceding siblings ...)
  2009-05-31 16:36 ` Alan Cox
@ 2009-05-31 20:04 ` Frans Pop
  2009-05-31 22:50   ` Andries E. Brouwer
  2009-06-01 12:59 ` Greg Freemyer
  7 siblings, 1 reply; 20+ messages in thread
From: Frans Pop @ 2009-05-31 20:04 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock, Al Viro

On Sunday 31 May 2009, Bartlomiej Zolnierkiewicz wrote:
> Since from the perspective of most users of recent systems, disabling
> Host Protected Area (HPA) can break vendor RAID formats, GPT partitions
> and risks corrupting firmware or overwriting vendor system recovery
> tools this patchset makes the IDE subsystem preserve HPA by default.

Kudos! I have to leave a real review of the patches to others more 
qualified than me, but where the previous patch set raised all sorts of 
questions for me, this just looks logical.

I conclude the following from reading the patches:
- a HPA is always at the end of a disk, correct?
- the only case where a user should notice a change after switching
  to 2.6.30 is if he has a partition that starts on or after the
  start of the HPA: such a partition will be ignored (with warning
  in dmesg); I guess that is reasonable as at least it will prevent
  "broken" partitions and is probably relatively uncommon.

Thanks for your continued work on this Bart.

Cheers,
FJP

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 20:04 ` Frans Pop
@ 2009-05-31 22:50   ` Andries E. Brouwer
  0 siblings, 0 replies; 20+ messages in thread
From: Andries E. Brouwer @ 2009-05-31 22:50 UTC (permalink / raw)
  To: Frans Pop
  Cc: Bartlomiej Zolnierkiewicz, linux-ide, Andries E. Brouwer,
	linux-kernel, Robert Hancock, Al Viro

On Sun, May 31, 2009 at 10:04:11PM +0200, Frans Pop wrote:

> I conclude the following from reading the patches:
> - a HPA is always at the end of a disk, correct?

I recall from the days of PARTIES and BEER the description
of the Address Offset Mode. If that is enabled, LBA 0 points
at the start of the Host Protected Area.

Andries




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
                   ` (6 preceding siblings ...)
  2009-05-31 20:04 ` Frans Pop
@ 2009-06-01 12:59 ` Greg Freemyer
  2009-06-01 13:06   ` Alan Cox
  7 siblings, 1 reply; 20+ messages in thread
From: Greg Freemyer @ 2009-06-01 12:59 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

On Sun, May 31, 2009 at 10:39 AM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>
<snip>
> Additionally HPA can also
> be disabled using the "nohpa" module parameter (i.e. "ide_core.nohpa=0.0"
> to disable HPA on /dev/hda).

If we use that parameter on older kernels, is it just ignored?  I'm
hoping I can just make that a default for my live CDs, etc..

Also, how is that handled in libata?  (ie. we typically want to force
exposing the HPA regardless of driver selection.)

Alan, given your emphatic ack, is libata likely to follow the same
model.  Any chance we could have a single boot param that handled both
drivers?  I mean instead of having 2 module params.

Thanks
Greg
-- 
Greg Freemyer
Head of EDD Tape Extraction and Processing team
Litigation Triage Solutions Specialist
http://www.linkedin.com/in/gregfreemyer
First 99 Days Litigation White Paper -
http://www.norcrossgroup.com/forms/whitepapers/99%20Days%20whitepaper.pdf

The Norcross Group
The Intersection of Evidence & Technology
http://www.norcrossgroup.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-05-31 15:34   ` Bartlomiej Zolnierkiewicz
@ 2009-06-01 13:02     ` Greg Freemyer
  0 siblings, 0 replies; 20+ messages in thread
From: Greg Freemyer @ 2009-06-01 13:02 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Andries E. Brouwer, linux-ide, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

On Sun, May 31, 2009 at 11:34 AM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
> On Sunday 31 May 2009 17:24:03 Andries E. Brouwer wrote:
>> On Sun, May 31, 2009 at 04:39:11PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>
>> [HPA stuff deleted]
>>
>> Hi Bartlomiej,
>>
>> Apart from HPA one also has DCO, which functions rather similarly.
>> (I have not checked whether the current kernel knows anything about DCO.)
>> Have you thought about DCO?
>> Probably the kernel should by default not touch any such setting.
>> Maybe it should be reported?
>> A user space utility might change this setting - thus a disk can change size.
>
> We leave DCO handling entirely to user-space currently.

And to the best of my knowledge user-space leaves it entirely to the kernel.

ie. There is no opensource Linux solution I know of.

Greg
-- 
Greg Freemyer
Head of EDD Tape Extraction and Processing team
Litigation Triage Solutions Specialist
http://www.linkedin.com/in/gregfreemyer
First 99 Days Litigation White Paper -
http://www.norcrossgroup.com/forms/whitepapers/99%20Days%20whitepaper.pdf

The Norcross Group
The Intersection of Evidence & Technology
http://www.norcrossgroup.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-06-01 12:59 ` Greg Freemyer
@ 2009-06-01 13:06   ` Alan Cox
  2009-06-01 22:00     ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Cox @ 2009-06-01 13:06 UTC (permalink / raw)
  To: Greg Freemyer
  Cc: Bartlomiej Zolnierkiewicz, linux-ide, Andries E. Brouwer,
	linux-kernel, Robert Hancock, Al Viro, Frans Pop

On Mon, 1 Jun 2009 08:59:29 -0400
Greg Freemyer <greg.freemyer@gmail.com> wrote:

> Alan, given your emphatic ack, is libata likely to follow the same
> model.  Any chance we could have a single boot param that handled both

If the patches get in then it makes no sense not to implement them
identically in libata.

> drivers?  I mean instead of having 2 module params.

No because the modules have different names so it will always be
ide_core.something and libata.something

Bartlomiej - thinking about this I question "nohpa=" because we get into
unneccessary negatives ide_core.hpa= is one less inversion to figure out.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] ide-gd: implement block device ->set_capacity method
  2009-05-31 14:39 ` [PATCH 3/4] ide-gd: implement block device ->set_capacity method Bartlomiej Zolnierkiewicz
@ 2009-06-01 21:32   ` Bartlomiej Zolnierkiewicz
  2009-06-02 18:55     ` Sergei Shtylyov
  0 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-01 21:32 UTC (permalink / raw)
  To: linux-ide
  Cc: Andries E. Brouwer, linux-kernel, Robert Hancock, Al Viro, Frans Pop

On Sunday 31 May 2009 16:39:31 Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide-gd: implement block device ->set_capacity method
> 
> * Use ->probed_capacity to store native device capacity for ATA disks.
> 
> * Add ->set_capacity method to struct ide_disk_ops.
> 
> * Implement disk device ->set_capacity method for ATA disks.
> 
> * Implement block device ->set_capacity method.
> 
> Together with the previous patch adding ->set_capacity block device
> method this allows automatic disabling of Host Protected Area (HPA)
> if any partitions overlapping HPA are detected.
> 
> Cc: Robert Hancock <hancockrwd@gmail.com>
> Cc: Frans Pop <elendil@planet.nl>
> Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

v2 interdiff

v2:
* Check if LBA and HPA are supported in ide_disk_set_capacity().

* According to the spec the SET MAX ADDRESS command shall be
  immediately preceded by a READ NATIVE MAX ADDRESS command.

* Add ide_disk_hpa_{get_native,set}_capacity() helpers.
---
 ide-disk.c |   58 +++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 15 deletions(-)

diff -u b/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- b/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -302,14 +302,12 @@
 	{ NULL,		NULL }
 };
 
-static void idedisk_check_hpa(ide_drive_t *drive)
+static u64 ide_disk_hpa_get_native_capacity(ide_drive_t *drive, int lba48)
 {
-	unsigned long long capacity, set_max;
-	int lba48 = ata_id_lba48_enabled(drive->id);
+	u64 capacity, set_max;
 
 	capacity = drive->capacity64;
-
-	set_max = idedisk_read_native_max_address(drive, lba48);
+	set_max  = idedisk_read_native_max_address(drive, lba48);
 
 	if (ide_in_drive_list(drive->id, hpa_list)) {
 		/*
@@ -320,6 +318,26 @@
 			set_max--;
 	}
 
+	return set_max;
+}
+
+static u64 ide_disk_hpa_set_capacity(ide_drive_t *drive, u64 set_max, int lba48)
+{
+	set_max = idedisk_set_max_address(drive, set_max, lba48);
+	if (set_max)
+		drive->capacity64 = set_max;
+
+	return set_max;
+}
+
+static void idedisk_check_hpa(ide_drive_t *drive)
+{
+	u64 capacity, set_max;
+	int lba48 = ata_id_lba48_enabled(drive->id);
+
+	capacity = drive->capacity64;
+	set_max  = ide_disk_hpa_get_native_capacity(drive, lba48);
+
 	if (set_max <= capacity)
 		return;
 
@@ -332,13 +350,10 @@
 			 capacity, sectors_to_MB(capacity),
 			 set_max, sectors_to_MB(set_max));
 
-	set_max = idedisk_set_max_address(drive, set_max, lba48);
-
-	if (set_max) {
-		drive->capacity64 = set_max;
+	set_max = ide_disk_hpa_set_capacity(drive, set_max, lba48);
+	if (set_max)
 		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
 				 drive->name);
-	}
 }
 
 static int ide_disk_get_capacity(ide_drive_t *drive)
@@ -399,12 +414,25 @@
 static u64 ide_disk_set_capacity(ide_drive_t *drive, u64 capacity)
 {
 	u64 set = min(capacity, drive->probed_capacity);
-	int lba48 = ata_id_lba48_enabled(drive->id);
-
-	capacity = idedisk_set_max_address(drive, set, lba48);
-	if (capacity)
-		drive->capacity64 = capacity;
+	u16 *id = drive->id;
+	int lba48 = ata_id_lba48_enabled(id);
 
+	if ((drive->dev_flags & IDE_DFLAG_LBA) == 0 ||
+	    ata_id_hpa_enabled(id) == 0)
+		goto out;
+
+	/*
+	 * according to the spec the SET MAX ADDRESS command shall be
+	 * immediately preceded by a READ NATIVE MAX ADDRESS command
+	 */
+	capacity = ide_disk_hpa_get_native_capacity(drive, lba48);
+	if (capacity == 0)
+		goto out;
+
+	set = ide_disk_hpa_set_capacity(drive, set, lba48);
+	if (set)
+		return set;
+out:
 	return drive->capacity64;
 }
 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] ide: preserve Host Protected Area by default
  2009-05-31 14:39 ` [PATCH 4/4] ide: preserve Host Protected Area by default Bartlomiej Zolnierkiewicz
@ 2009-06-01 21:33   ` Bartlomiej Zolnierkiewicz
  2009-06-02 19:12     ` Sergei Shtylyov
  0 siblings, 1 reply; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-01 21:33 UTC (permalink / raw)
  To: linux-ide
  Cc: Andries E. Brouwer, linux-kernel, Robert Hancock, Al Viro, Frans Pop

On Sunday 31 May 2009 16:39:39 Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: preserve Host Protected Area by default
> 
> From the perspective of most users of recent systems, disabling Host
> Protected Area (HPA) can break vendor RAID formats, GPT partitions and
> risks corrupting firmware or overwriting vendor system recovery tools.
> 
> Unfortunately the original (kernels < 2.6.30) behavior (unconditionally
> disabling HPA and using full disk capacity) was introduced at the time
> when the main use of HPA was to make the drive look small enough for the
> BIOS to allow the system to boot with large capacity drives.
> 
> Thus to allow the maximum compatibility with the existing setups (using
> HPA and partitioned with HPA disabled) we automatically disable HPA if
> any partitions overlapping HPA are detected.  Additionally HPA can also
> be disabled using the "nohpa" module parameter (i.e. "ide_core.nohpa=0.0"
> to disable HPA on /dev/hda).
> 
> While at it:
> - remove stale "idebus=" entry from Documentation/kernel-parameters.txt
> 
> Cc: Robert Hancock <hancockrwd@gmail.com>
> Cc: Frans Pop <elendil@planet.nl>
> Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> [patch description was based on input from Alan Cox and Frans Pop]
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

v2 interdiff

v2:
Fix ->resume HPA support.
---
 ide-disk.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff -u b/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- b/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -433,8 +433,11 @@
 		goto out;
 
 	set = ide_disk_hpa_set_capacity(drive, set, lba48);
-	if (set)
+	if (set) {
+		/* needed for ->resume to disable HPA */
+		drive->dev_flags |= IDE_DFLAG_NOHPA;
 		return set;
+	}
 out:
 	return drive->capacity64;
 }

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/4] partitions/ide: improve Host Protected Area handling
  2009-06-01 13:06   ` Alan Cox
@ 2009-06-01 22:00     ` Bartlomiej Zolnierkiewicz
  0 siblings, 0 replies; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-01 22:00 UTC (permalink / raw)
  To: Alan Cox
  Cc: Greg Freemyer, linux-ide, Andries E. Brouwer, linux-kernel,
	Robert Hancock, Al Viro, Frans Pop

On Monday 01 June 2009 15:06:51 Alan Cox wrote:
> On Mon, 1 Jun 2009 08:59:29 -0400
> Greg Freemyer <greg.freemyer@gmail.com> wrote:

[...]

> > drivers?  I mean instead of having 2 module params.
> 
> No because the modules have different names so it will always be
> ide_core.something and libata.something

Moreover "nohpa" is a per-device setting while "ignore_hpa" is a global one.

> Bartlomiej - thinking about this I question "nohpa=" because we get into
> unneccessary negatives ide_core.hpa= is one less inversion to figure out.

Actually in case of "nohpa" there are no unnecessary negatives because it
is really meant for disabling Host Protected Area only and sticking with
the current name allows us to use IDE's module parameters infrastructure
without adding some extra code/complexity.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] ide-gd: implement block device ->set_capacity method
  2009-06-01 21:32   ` Bartlomiej Zolnierkiewicz
@ 2009-06-02 18:55     ` Sergei Shtylyov
  2009-06-05 18:38       ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 20+ messages in thread
From: Sergei Shtylyov @ 2009-06-02 18:55 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>Subject: [PATCH] ide-gd: implement block device ->set_capacity method

>>* Use ->probed_capacity to store native device capacity for ATA disks.

>>* Add ->set_capacity method to struct ide_disk_ops.

>>* Implement disk device ->set_capacity method for ATA disks.

>>* Implement block device ->set_capacity method.

>>Together with the previous patch adding ->set_capacity block device
>>method this allows automatic disabling of Host Protected Area (HPA)
>>if any partitions overlapping HPA are detected.

>>Cc: Robert Hancock <hancockrwd@gmail.com>
>>Cc: Frans Pop <elendil@planet.nl>
>>Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
>>Cc: Al Viro <viro@zeniv.linux.org.uk>
>>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

> v2 interdiff

> v2:
> * Check if LBA and HPA are supported in ide_disk_set_capacity().

> * According to the spec the SET MAX ADDRESS command shall be
>   immediately preceded by a READ NATIVE MAX ADDRESS command.

> * Add ide_disk_hpa_{get_native,set}_capacity() helpers.

    One of them seem to me pretty useless...

> diff -u b/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
> --- b/drivers/ide/ide-disk.c
> +++ b/drivers/ide/ide-disk.c
> @@ -302,14 +302,12 @@
>  	{ NULL,		NULL }
>  };
>  
> -static void idedisk_check_hpa(ide_drive_t *drive)
> +static u64 ide_disk_hpa_get_native_capacity(ide_drive_t *drive, int lba48)

    Frankly speaking, I don't see what you've bought with factoring out this 
function...

>  {
> -	unsigned long long capacity, set_max;
> -	int lba48 = ata_id_lba48_enabled(drive->id);
> +	u64 capacity, set_max;
>  
>  	capacity = drive->capacity64;
> -
> -	set_max = idedisk_read_native_max_address(drive, lba48);
> +	set_max  = idedisk_read_native_max_address(drive, lba48);
>  
>  	if (ide_in_drive_list(drive->id, hpa_list)) {
>  		/*
> @@ -320,6 +318,26 @@
>  			set_max--;
>  	}
>  
> +	return set_max;
> +}
> +
> +static u64 ide_disk_hpa_set_capacity(ide_drive_t *drive, u64 set_max, int lba48)
> +{
> +	set_max = idedisk_set_max_address(drive, set_max, lba48);
> +	if (set_max)
> +		drive->capacity64 = set_max;
> +
> +	return set_max;
> +}
> +
> +static void idedisk_check_hpa(ide_drive_t *drive)
> +{
> +	u64 capacity, set_max;
> +	int lba48 = ata_id_lba48_enabled(drive->id);
> +
> +	capacity = drive->capacity64;
> +	set_max  = ide_disk_hpa_get_native_capacity(drive, lba48);
> +
>  	if (set_max <= capacity)
>  		return;
>  
> @@ -332,13 +350,10 @@
>  			 capacity, sectors_to_MB(capacity),
>  			 set_max, sectors_to_MB(set_max));
>  
> -	set_max = idedisk_set_max_address(drive, set_max, lba48);
> -
> -	if (set_max) {
> -		drive->capacity64 = set_max;
> +	set_max = ide_disk_hpa_set_capacity(drive, set_max, lba48);
> +	if (set_max)
>  		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
>  				 drive->name);
> -	}
>  }
>  
>  static int ide_disk_get_capacity(ide_drive_t *drive)
> @@ -399,12 +414,25 @@
>  static u64 ide_disk_set_capacity(ide_drive_t *drive, u64 capacity)
>  {
>  	u64 set = min(capacity, drive->probed_capacity);
> -	int lba48 = ata_id_lba48_enabled(drive->id);
> -
> -	capacity = idedisk_set_max_address(drive, set, lba48);
> -	if (capacity)
> -		drive->capacity64 = capacity;
> +	u16 *id = drive->id;
> +	int lba48 = ata_id_lba48_enabled(id);
>  
> +	if ((drive->dev_flags & IDE_DFLAG_LBA) == 0 ||
> +	    ata_id_hpa_enabled(id) == 0)
> +		goto out;
> +
> +	/*
> +	 * according to the spec the SET MAX ADDRESS command shall be
> +	 * immediately preceded by a READ NATIVE MAX ADDRESS command
> +	 */
> +	capacity = ide_disk_hpa_get_native_capacity(drive, lba48);

    Might as well just call idedisk_read_native_max_address() -- we don't 
need to lookup hpa_list[] if we're only checking whether we can read native 
max address or not afterwards:

> +	if (capacity == 0)
> +		goto out;
> +
> +	set = ide_disk_hpa_set_capacity(drive, set, lba48);
> +	if (set)
> +		return set;

    Why bother doing that again if the function itself does such check and 
sets drive->capacity64 to the returned value on success? It should be as 
simple as:

	ide_disk_hpa_set_capacity(drive, set, lba48);

> +out:
>  	return drive->capacity64;
>  }

    Label/gotos unnecessary in this case -- we have *return*...

MBR, Sergei

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] ide: preserve Host Protected Area by default
  2009-06-01 21:33   ` Bartlomiej Zolnierkiewicz
@ 2009-06-02 19:12     ` Sergei Shtylyov
  0 siblings, 0 replies; 20+ messages in thread
From: Sergei Shtylyov @ 2009-06-02 19:12 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>Subject: [PATCH] ide: preserve Host Protected Area by default

>>From the perspective of most users of recent systems, disabling Host
>>Protected Area (HPA) can break vendor RAID formats, GPT partitions and
>>risks corrupting firmware or overwriting vendor system recovery tools.

>>Unfortunately the original (kernels < 2.6.30) behavior (unconditionally
>>disabling HPA and using full disk capacity) was introduced at the time
>>when the main use of HPA was to make the drive look small enough for the
>>BIOS to allow the system to boot with large capacity drives.

>>Thus to allow the maximum compatibility with the existing setups (using
>>HPA and partitioned with HPA disabled) we automatically disable HPA if
>>any partitions overlapping HPA are detected.  Additionally HPA can also
>>be disabled using the "nohpa" module parameter (i.e. "ide_core.nohpa=0.0"
>>to disable HPA on /dev/hda).

>>While at it:
>>- remove stale "idebus=" entry from Documentation/kernel-parameters.txt

>>Cc: Robert Hancock <hancockrwd@gmail.com>
>>Cc: Frans Pop <elendil@planet.nl>
>>Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
>>Cc: Al Viro <viro@zeniv.linux.org.uk>
>>[patch description was based on input from Alan Cox and Frans Pop]
>>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> v2 interdiff

> v2:
> Fix ->resume HPA support.

> diff -u b/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
> --- b/drivers/ide/ide-disk.c
> +++ b/drivers/ide/ide-disk.c
> @@ -433,8 +433,11 @@
>  		goto out;
>  
>  	set = ide_disk_hpa_set_capacity(drive, set, lba48);
> -	if (set)
> +	if (set) {

    Well, this seems to be a useful check after all. :-)

> +		/* needed for ->resume to disable HPA */
> +		drive->dev_flags |= IDE_DFLAG_NOHPA;
>  		return set;
> +	}
>  out:
>  	return drive->capacity64;
>  }

MBR, Sergei

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] ide-gd: implement block device ->set_capacity method
  2009-06-02 18:55     ` Sergei Shtylyov
@ 2009-06-05 18:38       ` Bartlomiej Zolnierkiewicz
  0 siblings, 0 replies; 20+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-06-05 18:38 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock,
	Al Viro, Frans Pop

On Tuesday 02 June 2009 20:55:56 Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>Subject: [PATCH] ide-gd: implement block device ->set_capacity method
> 
> >>* Use ->probed_capacity to store native device capacity for ATA disks.
> 
> >>* Add ->set_capacity method to struct ide_disk_ops.
> 
> >>* Implement disk device ->set_capacity method for ATA disks.
> 
> >>* Implement block device ->set_capacity method.
> 
> >>Together with the previous patch adding ->set_capacity block device
> >>method this allows automatic disabling of Host Protected Area (HPA)
> >>if any partitions overlapping HPA are detected.
> 
> >>Cc: Robert Hancock <hancockrwd@gmail.com>
> >>Cc: Frans Pop <elendil@planet.nl>
> >>Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
> >>Cc: Al Viro <viro@zeniv.linux.org.uk>
> >>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> > v2 interdiff
> 
> > v2:
> > * Check if LBA and HPA are supported in ide_disk_set_capacity().
> 
> > * According to the spec the SET MAX ADDRESS command shall be
> >   immediately preceded by a READ NATIVE MAX ADDRESS command.
> 
> > * Add ide_disk_hpa_{get_native,set}_capacity() helpers.
> 
>     One of them seem to me pretty useless...
> 
> > diff -u b/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
> > --- b/drivers/ide/ide-disk.c
> > +++ b/drivers/ide/ide-disk.c
> > @@ -302,14 +302,12 @@
> >  	{ NULL,		NULL }
> >  };
> >  
> > -static void idedisk_check_hpa(ide_drive_t *drive)
> > +static u64 ide_disk_hpa_get_native_capacity(ide_drive_t *drive, int lba48)
> 
>     Frankly speaking, I don't see what you've bought with factoring out this 
> function...

Saner abstractions and increased code clarity.

[ Makes long-term maintenance easier. ]

> >  {
> > -	unsigned long long capacity, set_max;
> > -	int lba48 = ata_id_lba48_enabled(drive->id);
> > +	u64 capacity, set_max;
> >  
> >  	capacity = drive->capacity64;
> > -
> > -	set_max = idedisk_read_native_max_address(drive, lba48);
> > +	set_max  = idedisk_read_native_max_address(drive, lba48);
> >  
> >  	if (ide_in_drive_list(drive->id, hpa_list)) {
> >  		/*
> > @@ -320,6 +318,26 @@
> >  			set_max--;
> >  	}
> >  
> > +	return set_max;
> > +}
> > +
> > +static u64 ide_disk_hpa_set_capacity(ide_drive_t *drive, u64 set_max, int lba48)
> > +{
> > +	set_max = idedisk_set_max_address(drive, set_max, lba48);
> > +	if (set_max)
> > +		drive->capacity64 = set_max;
> > +
> > +	return set_max;
> > +}
> > +
> > +static void idedisk_check_hpa(ide_drive_t *drive)
> > +{
> > +	u64 capacity, set_max;
> > +	int lba48 = ata_id_lba48_enabled(drive->id);
> > +
> > +	capacity = drive->capacity64;
> > +	set_max  = ide_disk_hpa_get_native_capacity(drive, lba48);
> > +
> >  	if (set_max <= capacity)
> >  		return;
> >  
> > @@ -332,13 +350,10 @@
> >  			 capacity, sectors_to_MB(capacity),
> >  			 set_max, sectors_to_MB(set_max));
> >  
> > -	set_max = idedisk_set_max_address(drive, set_max, lba48);
> > -
> > -	if (set_max) {
> > -		drive->capacity64 = set_max;
> > +	set_max = ide_disk_hpa_set_capacity(drive, set_max, lba48);
> > +	if (set_max)
> >  		printk(KERN_INFO "%s: Host Protected Area disabled.\n",
> >  				 drive->name);
> > -	}
> >  }
> >  
> >  static int ide_disk_get_capacity(ide_drive_t *drive)
> > @@ -399,12 +414,25 @@
> >  static u64 ide_disk_set_capacity(ide_drive_t *drive, u64 capacity)
> >  {
> >  	u64 set = min(capacity, drive->probed_capacity);
> > -	int lba48 = ata_id_lba48_enabled(drive->id);
> > -
> > -	capacity = idedisk_set_max_address(drive, set, lba48);
> > -	if (capacity)
> > -		drive->capacity64 = capacity;
> > +	u16 *id = drive->id;
> > +	int lba48 = ata_id_lba48_enabled(id);
> >  
> > +	if ((drive->dev_flags & IDE_DFLAG_LBA) == 0 ||
> > +	    ata_id_hpa_enabled(id) == 0)
> > +		goto out;
> > +
> > +	/*
> > +	 * according to the spec the SET MAX ADDRESS command shall be
> > +	 * immediately preceded by a READ NATIVE MAX ADDRESS command
> > +	 */
> > +	capacity = ide_disk_hpa_get_native_capacity(drive, lba48);
> 
>     Might as well just call idedisk_read_native_max_address() -- we don't 
> need to lookup hpa_list[] if we're only checking whether we can read native 
> max address or not afterwards:

ditto :)

None of this code is performance critical or adds significant footprint to
the resulting binary size..

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] partitions: add ->set_capacity block device method
  2009-05-31 14:39 ` [PATCH 2/4] partitions: add ->set_capacity block device method Bartlomiej Zolnierkiewicz
@ 2009-06-06  8:42   ` Al Viro
  0 siblings, 0 replies; 20+ messages in thread
From: Al Viro @ 2009-06-06  8:42 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-ide, Andries E. Brouwer, linux-kernel, Robert Hancock, Frans Pop

On Sun, May 31, 2009 at 04:39:24PM +0200, Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] partitions: add ->set_capacity block device method
> 
> * Add ->set_capacity block device method and use it in rescan_partitions()
>   to attempt enabling native capacity of the device upon detecting the
>   partition which exceeds device capacity.
> 
> * Add GENHD_FL_NATIVE_CAPACITY flag to try limit attempts of enabling
>   native capacity during partition scan.
> 
> Together with the consecutive patch implementing ->set_capacity method in
> ide-gd device driver this allows automatic disabling of Host Protected Area
> (HPA) if any partitions overlapping HPA are detected.
> 
> Cc: Robert Hancock <hancockrwd@gmail.com>
> Cc: Frans Pop <elendil@planet.nl>
> Cc: "Andries E. Brouwer" <Andries.Brouwer@cwi.nl>
> Cc: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Al Viro <viro@zeniv.linux.org.uk>

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2009-06-06  8:42 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-31 14:39 [PATCH 0/4] partitions/ide: improve Host Protected Area handling Bartlomiej Zolnierkiewicz
2009-05-31 14:39 ` [PATCH 1/4] partitions: warn about the partition exceeding device capacity Bartlomiej Zolnierkiewicz
2009-05-31 14:39 ` [PATCH 2/4] partitions: add ->set_capacity block device method Bartlomiej Zolnierkiewicz
2009-06-06  8:42   ` Al Viro
2009-05-31 14:39 ` [PATCH 3/4] ide-gd: implement block device ->set_capacity method Bartlomiej Zolnierkiewicz
2009-06-01 21:32   ` Bartlomiej Zolnierkiewicz
2009-06-02 18:55     ` Sergei Shtylyov
2009-06-05 18:38       ` Bartlomiej Zolnierkiewicz
2009-05-31 14:39 ` [PATCH 4/4] ide: preserve Host Protected Area by default Bartlomiej Zolnierkiewicz
2009-06-01 21:33   ` Bartlomiej Zolnierkiewicz
2009-06-02 19:12     ` Sergei Shtylyov
2009-05-31 15:24 ` [PATCH 0/4] partitions/ide: improve Host Protected Area handling Andries E. Brouwer
2009-05-31 15:34   ` Bartlomiej Zolnierkiewicz
2009-06-01 13:02     ` Greg Freemyer
2009-05-31 16:36 ` Alan Cox
2009-05-31 20:04 ` Frans Pop
2009-05-31 22:50   ` Andries E. Brouwer
2009-06-01 12:59 ` Greg Freemyer
2009-06-01 13:06   ` Alan Cox
2009-06-01 22:00     ` Bartlomiej Zolnierkiewicz

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