Re: [PATCH 3/4] ide-gd: implement block device ->set_capacity method

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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