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