| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 5 Jun 2009 20:38:02 +0200
From: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To: Sergei Shtylyov <sshtylyov@...mvista.com>
Cc: linux-ide@...r.kernel.org,
"Andries E. Brouwer" <Andries.Brouwer@....nl>,
linux-kernel@...r.kernel.org,
Robert Hancock <hancockrwd@...il.com>,
Al Viro <viro@...iv.linux.org.uk>,
Frans Pop <elendil@...net.nl>
Subject: Re: [PATCH 3/4] ide-gd: implement block device ->set_capacity method
On Tuesday 02 June 2009 20:55:56 Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>From: Bartlomiej Zolnierkiewicz <bzolnier@...il.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@...il.com>
> >>Cc: Frans Pop <elendil@...net.nl>
> >>Cc: "Andries E. Brouwer" <Andries.Brouwer@....nl>
> >>Cc: Al Viro <viro@...iv.linux.org.uk>
> >>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.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..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists