only use 48-bit lba when necessary
PATCH 1/1 only use 48-bit lba when necessary
Hi,
48-bit lba has a non-significant overhead (twice the outb's, 12 instead of 6 per command), so it makes sense to use 28-bit lba commands whenever we can.
Patch is against 2.5.66-BK.
===== drivers/ide/ide-disk.c 1.36 vs edited =====
drivers/ide/ide-disk.c +24 -8
@@ -367,12 +367,15 @@static ide_startstop_t do_rw_disk (ide_drive_t *drive,struct request *rq,sector_t block) {ide_hwif_t *hwif =HWIF (drive); -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ; +u8 lba48 =0 ;task_ioreg_t command =WIN_NOP ;ata_nsector_t nsectors; nsectors.all = (u16 ) rq->nr_sectors; +if (drive->addressing ==1 && block >0xfffffff ) + lba48 =1 ; +if (driver_blocked)panic ("Request while ide driver is blocked?" );
@@ -392,7 +395,7 @@ hwif->OUTB (drive->ctl,IDE_CONTROL_REG );if (drive->select.b.lba) { -if (drive->addressing ==1 ) { +if (lba48) {task_ioreg_t tasklets[10 ];if (blk_rq_tagged (rq)) {
@@ -502,6 +505,7 @@ command = ((drive->mult_count) ? ((lba48) ?WIN_MULTREAD_EXT :WIN_MULTREAD ) : ((lba48) ?WIN_READ_EXT :WIN_READ )); +ide_execute_command (drive, command, &read_intr,WAIT_CMD ,NULL );return ide_started; }else if (rq_data_dir (rq) ==WRITE ) {
@@ -577,6 +581,11 @@*/ static ide_startstop_t do_rw_disk (ide_drive_t *drive,struct request *rq,sector_t block) { +int lba48bit =0 ; + +if (drive->addressing ==1 && block >0xfffffff ) + lba48bit =1 ; +BUG_ON (drive->blocked);if (!blk_fs_request (rq)) {blk_dump_rq_flags (rq,"do_rw_disk - bad command" );
@@ -602,7 +611,7 @@return ide_started; } -if (drive->addressing ==1 )/* 48-bit LBA */ +if (lba48bit && block >0xfffffff )return lba_48_rw_disk (drive, rq, (unsigned long long ) block);if (drive->select.b.lba)/* 28-bit LBA */ return lba_28_rw_disk (drive, rq, (unsigned long ) block);
@@ -611,9 +620,13 @@return chs_rw_disk (drive, rq, (unsigned long ) block); } -static task_ioreg_t get_command (ide_drive_t *drive,int cmd) +static task_ioreg_t get_command (ide_drive_t *drive,struct request *rq) { -int lba48bit = (drive->addressing ==1 ) ?1 :0 ; +int cmd =rq_data_dir (rq); +int lba48bit =0 ; + +if (drive->addressing ==1 && rq->sector >0xfffffff ) + lba48bit =1 ;if ((cmd ==READ ) && drive->using_tcq)return lba48bit ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;
@@ -640,7 +653,7 @@ide_task_t args;int sectors;ata_nsector_t nsectors; -task_ioreg_t command =get_command (drive,rq_data_dir (rq)); +task_ioreg_t command =get_command (drive, rq);unsigned int track = (block / drive->sect);unsigned int sect = (block % drive->sect) +1 ;unsigned int head = (track % drive->head);
@@ -672,6 +685,7 @@ args.tfRegister[IDE_SELECT_OFFSET ] |= drive->select.all; args.tfRegister[IDE_COMMAND_OFFSET ] = command; args.command_type =ide_cmd_type_parser (&args); + args.addressing =0 ; args.rq = (struct request *) rq; rq->special = (ide_task_t *)&args;return do_rw_taskfile (drive, &args);
@@ -682,7 +696,7 @@ide_task_t args;int sectors;ata_nsector_t nsectors; -task_ioreg_t command =get_command (drive,rq_data_dir (rq)); +task_ioreg_t command =get_command (drive, rq); nsectors.all = (u16 ) rq->nr_sectors;
@@ -710,6 +724,7 @@ args.tfRegister[IDE_SELECT_OFFSET ] |= drive->select.all; args.tfRegister[IDE_COMMAND_OFFSET ] = command; args.command_type =ide_cmd_type_parser (&args); + args.addressing =0 ; args.rq = (struct request *) rq; rq->special = (ide_task_t *)&args;return do_rw_taskfile (drive, &args);
@@ -726,7 +741,7 @@ide_task_t args;int sectors;ata_nsector_t nsectors; -task_ioreg_t command =get_command (drive,rq_data_dir (rq)); +task_ioreg_t command =get_command (drive, rq); nsectors.all = (u16 ) rq->nr_sectors;
@@ -762,6 +777,7 @@ args.hobRegister[IDE_SELECT_OFFSET_HOB ] = drive->select.all; args.hobRegister[IDE_CONTROL_OFFSET_HOB ]= (drive->ctl|0x80 ); args.command_type =ide_cmd_type_parser (&args); + args.addressing =1 ; args.rq = (struct request *) rq; rq->special = (ide_task_t *)&args;return do_rw_taskfile (drive, &args);
===== drivers/ide/ide-dma.c 1.13 vs edited =====
drivers/ide/ide-dma.c +11 -5
@@ -653,7 +653,7 @@ide_hwif_t *hwif =HWIF (drive);struct request *rq =HWGROUP (drive)->rq;unsigned int reading =1 <<3 ; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ; +u8 lba48 =0 ;task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -663,11 +663,14 @@if (drive->media != ide_disk)return 0 ; +if (drive->addressing ==1 && rq->sector >0xfffffff ) + lba48 =1 ; + command = (lba48) ?WIN_READDMA_EXT :WIN_READDMA ; - +if (drive->vdma) command = (lba48) ?WIN_READ_EXT :WIN_READ ; - +if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
@@ -685,7 +688,7 @@ide_hwif_t *hwif =HWIF (drive);struct request *rq =HWGROUP (drive)->rq;unsigned int reading =0 ; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ; +u8 lba48 =0 ;task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
@@ -695,10 +698,13 @@if (drive->media != ide_disk)return 0 ; +if (drive->addressing ==1 && rq->sector >0xfffffff ) + lba48 =1 ; + command = (lba48) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ;if (drive->vdma) command = (lba48) ?WIN_WRITE_EXT :WIN_WRITE ; - +if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
===== drivers/ide/ide-taskfile.c 1.14 vs edited =====
drivers/ide/ide-taskfile.c +5 -5
@@ -147,7 +147,7 @@ide_hwif_t *hwif =HWIF (drive);task_struct_t *taskfile = (task_struct_t *) task->tfRegister;hob_struct_t *hobfile = (hob_struct_t *) task->hobRegister; -u8 HIHI = (drive->addressing ==1 ) ?0xE0 :0xEF ; +u8 HIHI = task->addressing ==1 ?0xE0 :0xEF ;#ifdef CONFIG_IDE_TASK_IOCTL_DEBUG void debug_taskfile (drive ,task );
@@ -160,7 +160,7 @@ }SELECT_MASK (drive,0 ); -if (drive->addressing ==1 ) { +if (task->addressing ==1 ) { hwif->OUTB (hobfile->feature,IDE_FEATURE_REG ); hwif->OUTB (hobfile->sector_count,IDE_NSECTOR_REG ); hwif->OUTB (hobfile->sector_number,IDE_SECTOR_REG );
@@ -332,7 +332,7 @@ args->tfRegister[IDE_STATUS_OFFSET ] = stat;if ((drive->id->command_set_2 &0x0400 ) && (drive->id->cfs_enable_2 &0x0400 ) && - (drive->addressing ==1 )) { + (args->addressing ==1 )) { hwif->OUTB (drive->ctl|0x80 ,IDE_CONTROL_REG_HOB ); args->hobRegister[IDE_FEATURE_OFFSET_HOB ] = hwif->INB (IDE_FEATURE_REG ); args->hobRegister[IDE_NSECTOR_OFFSET_HOB ] = hwif->INB (IDE_NSECTOR_REG );
@@ -1795,13 +1795,13 @@*/ if (task->tf_out_flags.all ==0 ) { task->tf_out_flags.all =IDE_TASKFILE_STD_OUT_FLAGS ; -if (drive->addressing ==1 ) +if (task->addressing ==1 ) task->tf_out_flags.all |= (IDE_HOB_STD_OUT_FLAGS <<8 ); }if (task->tf_in_flags.all ==0 ) { task->tf_in_flags.all =IDE_TASKFILE_STD_IN_FLAGS ; -if (drive->addressing ==1 ) +if (task->addressing ==1 ) task->tf_in_flags.all |= (IDE_HOB_STD_IN_FLAGS <<8 ); }
===== include/linux/ide.h 1.42 vs edited =====
include/linux/ide.h +1 -0
@@ -1408,6 +1408,7 @@ide_reg_valid_t tf_in_flags;int data_phase;int command_type; +int addressing;/* 1 for 48-bit */ ide_pre_handler_t *prehandler;ide_handler_t *handler;ide_post_handler_t *posthandler;
Signature · 1 line
Jens Axboe
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"jens" == Jens Axboe <[email protected]> writes:
jens> Hi,
jens> 48-bit lba has a non-significant overhead (twice the outb's, 12 instead jens> of 6 per command), so it makes sense to use 28-bit lba commands whenever jens> we can.
jens> Patch is against 2.5.66-BK.
jens> ===== drivers/ide/ide-disk.c 1.36 vs edited =====
jens> --- 1.36/drivers/ide/ide-disk.c Wed Mar 26 21:23:01 2003
jens> +++ edited/drivers/ide/ide-disk.c Fri Apr 4 14:18:41 2003
jens> @@ -367,12 +367,15 @@
jens> static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, sector_t block) jens> {
jens> ide_hwif_t *hwif = HWIF(drive);
jens> - u8 lba48 = (drive->addressing == 1) ? 1 : 0;
jens> + u8 lba48 = 0;
jens> task_ioreg_t command = WIN_NOP;
jens> ata_nsector_t nsectors;
jens> nsectors.all = (u16) rq->nr_sectors;
jens> + if (drive->addressing == 1 && block > 0xfffffff)
jens> + lba48 = 1;
jens> +
lba48 = (drive->addressing == 1) && (block > 0xfffffff);
should do the trick.
jens> + int lba48bit = 0;
jens> +
jens> + if (drive->addressing == 1 && block > 0xfffffff)
jens> + lba48bit = 1;
see above (more cases of that stripped).
jens> + if (lba48bit && block > 0xfffffff)
that test should be equivalent to:
if (lba48bit)
(at least the cvs copy of that function don't modify block isnce the definition of lba48bit).
Talking about consistency, wouldn't be better to use always the same name, lba48 or lba48bit?
Later, Juan.
Signature · 2 lines
In theory, practice and theory are the same, but in practice they are different -- Larry McVoy
Quoted 23 lines · Juan Quintela
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"jens" == Jens Axboe <[email protected]> writes:
jens> Hi,
jens> 48-bit lba has a non-significant overhead (twice the outb's, 12 instead jens> of 6 per command), so it makes sense to use 28-bit lba commands whenever jens> we can.jens> Patch is against 2.5.66-BK.
jens> =====
drivers/ide/ide-disk.c1.36 vs edited =====
jens> ---1.36/drivers/ide/ide-disk.cWed Mar 26 21:23:01 2003
jens> +++edited/drivers/ide/ide-disk.cFri Apr 4 14:18:41 2003
jens> @@ -367,12 +367,15 @@
jens> staticide_startstop_tdo_rw_disk(ide_drive_t*drive, struct request *rq,sector_tblock) jens> {
jens>ide_hwif_t*hwif = HWIF(drive);
jens> - u8 lba48 = (drive->addressing== 1) ? 1 : 0;
jens> + u8 lba48 = 0;
jens>task_ioreg_tcommand =WIN_NOP;
jens>ata_nsector_tnsectors;jens> nsectors.all = (u16)
rq->nr_sectors;
Quoted 7 lines
jens> + if (
drive->addressing== 1 && block >0xfffffff)
jens> + lba48 = 1;
jens> +lba48 = (
drive->addressing== 1) && (block >0xfffffff);should do the trick.
I'm not going to use such nonsense, sorry. The spelled out versions are a lot more readable. The command ?: constructs used in ide-disk are a joke, imo.
Quoted 5 lines
jens> + if (lba48bit && block >
0xfffffff)that test should be equivalent to:
if (lba48bit)
Yes that is correct, that should be changed.
Quoted 2 lines
Talking about consistency, wouldn't be better to use always the same name, lba48 or lba48bit?
There was no consistency to begin with, if anything my patch is consistent with the inconsistent source :)
Signature · 1 line
Jens Axboe
Hi
jens> + if (drive->addressing == 1 && block > 0xfffffff)
jens> + lba48 = 1;
jens> +
Quoted 4 lines
Quoted 4 lines
lba48 = (
drive->addressing== 1) && (block >0xfffffff);should do the trick.
jens> I'm not going to use such nonsense, sorry. The spelled out versions are jens> a lot more readable. The command ?: constructs used in ide-disk are a jens> joke, imo.
Read it again, please. Told me wehre are the ?: command.
Reason is that:
if (expr)
var = true;
else
var = false;
is always a bad construct.
var = expr;
is a better construct to express that meaning.
And yes, your is a variation of the same theme:
var = false;
if (expr)
var = true;
Later, Juan "who also didn't like ?: operator"
Signature · 2 lines
In theory, practice and theory are the same, but in practice they are different -- Larry McVoy
Quoted 9 lines · Juan Quintela
Hi
jens> + if (
drive->addressing== 1 && block >0xfffffff)
jens> + lba48 = 1;
jens> +Quoted 3 lines
Quoted 3 lines
lba48 = (
drive->addressing== 1) && (block >0xfffffff);
Quoted 7 lines
Quoted 1 line
Quoted 1 line
should do the trick.
jens> I'm not going to use such nonsense, sorry. The spelled out versions are jens> a lot more readable. The command ?: constructs used in ide-disk are a jens> joke, imo.
Read it again, please. Told me wehre are the ?: command.
Oh, you are right. It was the one-liner style that threw me off.
Quoted 11 lines
Reason is that:
if (expr)
var = true;
else
var = false;is always a bad construct.
var = expr;
Quoted 7 lines
is a better construct to express that meaning.
And yes, your is a variation of the same theme:
var = false;
if (expr)
var = true;
Yes, but mine is more readable. IMO of course, that's the way it is with styles.
Signature · 1 line
Jens Axboe
Quoted 13 lines
Quoted 13 lines
Reason is that:
if (expr)
var = true;
else
var = false;is always a bad construct.
var = expr;
is a better construct to express that meaning.
Quoted 8 lines
Quoted 5 lines
And yes, your is a variation of the same theme:
var = false;
if (expr)
var = true;Yes, but mine is more readable. IMO of course, that's the way it is with styles.
if (!expr || expr) var=(!expr ? !expr : expr);
:-)
John.
Quoted 1 line · Jens Axboe
48-bit lba has a non-significant overhead (twice the outb's, 12 instead
s/non-// ?
drivers/ide/ide-disk.c line 377
Quoted 6 lines
of 6 per command), so it makes sense to use 28-bit lba commands whenever we can.
drivers/ide/ide-disk.c +2
-0+if (drive->addressing ==1 && block >0xfffffff ) + lba48 =1 ;
Hmm. I wonder whether we should be more cautious, and ask for lba48 as soon as some part of the interval is past this limit. (say, block+nsectors > 0xfffffff)
I don't know whether the standard spells out what happens at the boundary, but for example the LBA low/mid/high, DEV is required to contain the sector number at the place the error occurred, and that is possible only if one stays below the 28-byte sector limit.
Andries
Quoted 5 lines · Andries Brouwer
Quoted 1 line · Jens Axboe
48-bit lba has a non-significant overhead (twice the outb's, 12 instead
s/non-// ?
Of course :)
Quoted 11 lines
drivers/ide/ide-disk.c line 377
Quoted 6 lines
of 6 per command), so it makes sense to use 28-bit lba commands whenever we can.
drivers/ide/ide-disk.c +2
-0+if (drive->addressing ==1 && block >0xfffffff ) + lba48 =1 ;Hmm. I wonder whether we should be more cautious, and ask for lba48 as soon as some part of the interval is past this limit. (say, block+nsectors >
0xfffffff)
Quoted 4 lines
I don't know whether the standard spells out what happens at the boundary, but for example the LBA low/mid/high, DEV is required to contain the sector number at the place the error occurred, and that is possible only if one stays below the 28-byte sector limit.
That might not be a bad idea, just to be on the safe side. I'll do that.
Signature · 1 line
Jens Axboe