2.5 ide 48-bit usage
PATCH 1/1 2.5 ide 48-bit usage
Hi,
I did a patch for 2.4 some weeks ago that added (what I consider) proper 48-bit lba usage to ide-disk. Basically that consists of:
- Only using 48-bit commands when necessary
- When we can support 48-bit commands, take advantage of them by using larger requests than the 64k we are currently limiting ide-disk to.
Here's the same thing for 2.5 in its final version (equiv patch exists in 2.4 ppc tree), I've been using it on my laptop for weeks. It defaults to 1024kb request size, maybe we'd want to cut that to 512kb or something in that range.
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1091 -> 1.1092
# drivers/ide/ide-taskfile.c 1.16 -> 1.17
# drivers/ide/ppc/pmac.c 1.10 -> 1.11
# include/linux/ide.h 1.48 -> 1.49
# drivers/ide/setup-pci.c 1.13 -> 1.14
# drivers/ide/ide-disk.c 1.40 -> 1.41
# drivers/ide/ide-dma.c 1.13 -> 1.14
# drivers/ide/pci/pdc202xx_old.c 1.13 -> 1.14
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/07 [email protected] 1.1092
# Proper 48-bit lba support
# --------------------------------------------
#
drivers/ide/ide-disk.c +24 -9
@@ -358,7 +358,7 @@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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;ata_nsector_t nsectors;
@@ -383,7 +383,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)) {
@@ -593,7 +593,7 @@return ide_started; } -if (drive->addressing ==1 )/* 48-bit LBA */ +if (rq_lba48 (rq))/* 48-bit LBA */ 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);
@@ -602,9 +602,10 @@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 lba48bit =rq_lba48 (rq); +int cmd =rq_data_dir (rq);if ((cmd ==READ ) && drive->using_tcq)return lba48bit ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;
@@ -631,7 +632,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);
@@ -663,6 +664,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);
@@ -673,7 +675,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;
@@ -701,6 +703,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);
@@ -717,7 +720,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;
@@ -753,6 +756,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);
@@ -1479,7 +1483,7 @@static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +return probe_lba_addressing (drive, arg); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1565,6 +1569,17 @@ } (void )probe_lba_addressing (drive,1 ); + +if (drive->addressing ==1 ) { +ide_hwif_t *hwif =HWIF (drive); +int max_s =2048 ; + +if (max_s > hwif->rqsize) + max_s = hwif->rqsize; + +blk_queue_max_sectors (&drive->queue, max_s); +printk ("%s: max request size: %dKiB\n" , drive->name, max_s /2 ); + }/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-dma.c +2 -2
@@ -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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -685,7 +685,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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
drivers/ide/ide-taskfile.c +5 -5
@@ -138,7 +138,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 );
@@ -151,7 +151,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 );
@@ -241,7 +241,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 );
@@ -1611,13 +1611,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 ); }
drivers/ide/pci/pdc202xx_old.c +6 -3
@@ -535,8 +535,9 @@static int pdc202xx_old_ide_dma_begin (ide_drive_t *drive) { -if (drive->addressing ==1 ) { -struct request *rq =HWGROUP (drive)->rq; +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// struct pci_dev *dev = hwif->pci_dev; // unsgned long high_16 = pci_resource_start(dev, 4);
@@ -557,7 +558,9 @@static int pdc202xx_old_ide_dma_end (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4); unsigned long high_16 = hwif->dma_master;
drivers/ide/ppc/pmac.c +2 -4
@@ -1240,7 +1240,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1272,7 +1271,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_READDMA_EXT :WIN_READDMA ; + command =rq_lba48 (rq) ?WIN_READDMA_EXT :WIN_READDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
@@ -1293,7 +1292,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1325,7 +1323,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
drivers/ide/setup-pci.c +3 -0
@@ -658,6 +658,9 @@*/ d->init_hwif (hwif); +if (!hwif->rqsize) + hwif->rqsize =65535 ; +/* * This is in the wrong place. The driver may * do set up based on the autotune value and this
include/linux/ide.h +7 -0
@@ -846,6 +846,12 @@bio_kunmap_irq (buffer, flags); } +/* +* must be addressed with 48-bit lba +*/ +#define rq_lba48 (rq) \ + (((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256) +#define IDE_CHIPSET_PCI_MASK \ ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))#define IDE_CHIPSET_IS_PCI (c) ((IDE_CHIPSET_PCI_MASK >> (c)) & 1)
@@ -1387,6 +1393,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 3 lines · Jens Axboe
I did a patch for 2.4 some weeks ago that added (what I consider) proper 48-bit lba usage to ide-disk.
Ok, let me disagree a bit.
At least if I read the patch correctly, theer's no way for upper layers to say "I want 48-bit addressing" - it's just turned on automatically for high sectors (or big transfers).
Well, you can mark the drive itself as wanting 48-bit transfers, but you can't do it on a per-request basis.
And I think this is 100% equivalent to the 6 vs 10 vs 16-byte SCSI command issue, and I really think it should b esolved the same way. Namely, you should be able to (on a "struct request" level) explicitly say that you want the big requests, and then the default prep_fn() would do roughly what you do now by default.
That way something like a SG_IO interface could force one mode or the other on a per-request basis.
Comments?
Linus
Quoted 11 lines · Linus Torvalds
Quoted 3 lines · Jens Axboe
I did a patch for 2.4 some weeks ago that added (what I consider) proper 48-bit lba usage to ide-disk.
Ok, let me disagree a bit.
At least if I read the patch correctly, theer's no way for upper layers to say "I want 48-bit addressing" - it's just turned on automatically for high sectors (or big transfers).
Correct.
Quoted 8 lines
Well, you can mark the drive itself as wanting 48-bit transfers, but you can't do it on a per-request basis.
And I think this is 100% equivalent to the 6 vs 10 vs 16-byte SCSI command issue, and I really think it should b esolved the same way. Namely, you should be able to (on a "struct request" level) explicitly say that you want the big requests, and then the default
prep_fn()would do roughly what you do now by default.
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
Quoted 2 lines
That way something like a
SG_IOinterface could force one mode or the other on a per-request basis.
Doesn't make a lot of sense in this case I think, because the command associated with the SG_IO request would implicitly be either a 28-bit or 48-bit command based on the opcode of the request.
Quoted 1 line
Comments?
You haven't really convinced me yet. Is there some hardware out there that requires to be addressed in 48-bit mode? If that is the case, then yes a bit is missing to fully support that. We'd probably have a forced addressing flag in the hwif, and the drive->addressing would inherit that. So instead of
const int lba48 = rq_lba48(rq);
it would be
const int lba48 = rq_lba48(rq) || drive->addressing == FORCED_48;
(forgive the ugliness of the above construct, it's just meant to below operation of it, rq_lba48 would probably just take both drive and rq as parameter).
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
Also, if you want to re-create some particular IO pattern for debugging, you may want to explicitly use 48-bit addressing.
Linus
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Jens Axboe
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
For that, a forced 48-bit hwif->addressing inherited by drives will suffice. And I agree, we should have that.
See, the logic in the kernel right now is just to check whether the drive supports 48-bit commands. If it does, we use them like we would 28-bit commands. We eat the extra overhead, and do nothing to take advantage of it (except using big drives, of course). What's the logic in that?!
Quoted 2 lines
Also, if you want to re-create some particular IO pattern for debugging, you may want to explicitly use 48-bit addressing.
Then you just recreate those commands, there's absolutely no need for anything special in this case. And the current patch neither explicitly allows or disallows anything that the stock kernel doesn't.
If you are doing that from userspace by sending in taskfiles with the appropriate commands, then you just create the commands like you want them. rq_lba48() just checks whether file system requests should be executed with 28 or 48-bit commands. If you send in taskfiles (your SG_IO example), you have complete control over this already.
Signature · 1 line
Jens Axboe
Quoted 7 lines · Jens Axboe
Quoted 4 lines
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
For that, a forced 48-bit
hwif->addressinginherited by drives will suffice. And I agree, we should have that.
No no no.
You definitely do NOT want to set "hwif->addressing" to 1 before you've tested whether it even _works_.
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Only after it has verified that 48-bit addressing does work should it set the global flag.
Quoted 5 lines
If you are doing that from userspace by sending in taskfiles with the appropriate commands, then you just create the commands like you want them.
rq_lba48()just checks whether file system requests should be executed with 28 or 48-bit commands. If you send in taskfiles (yourSG_IOexample), you have complete control over this already.
That may well be sufficient.
Linus
Quoted 7 lines · Linus Torvalds
Quoted 5 lines · Jens Axboe
Quoted 4 lines
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
Quoted 7 lines
Quoted 2 lines
For that, a forced 48-bit
hwif->addressinginherited by drives will suffice. And I agree, we should have that.No no no.
You definitely do NOT want to set "
hwif->addressing" to 1 before you've tested whether it even _works_.
Well duh, of course not. Whether a given request is executed in 48-bit or not is a check that _includes_ drive capabilities too of course.
Quoted 5 lines
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Then it just puts a taskfile request on the request queue and lets it reach the drive, nicely syncronized with the other requests. There's no need to toggle any special bits for that.
Quoted 2 lines
Only after it has verified that 48-bit addressing does work should it set the global flag.
Sounds fine.
Signature · 1 line
Jens Axboe
Quoted 11 lines · Jens Axboe
Quoted 10 lines · Linus Torvalds
Quoted 7 lines · Jens Axboe
Quoted 4 lines
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
For that, a forced 48-bit
hwif->addressinginherited by drives will suffice. And I agree, we should have that.
Quoted 7 lines
Quoted 4 lines
No no no.
You definitely do NOT want to set "
hwif->addressing" to 1 before you've tested whether it even _works_.Well duh, of course not. Whether a given request is executed in 48-bit or not is a check that _includes_ drive capabilities too of course.
Yeah, we test drive capabilities properly in idedisk_setup(), but Linus is right speaking about _hwif_ capabilities.
Jens you your patch sets hwif->rqsize to 65535 in setup-pci.c for all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check for hwif->addressing and if true set rqsize to 65536 (not 65535) and not in IDE PCI code but in ide_init_queue() in ide-probe.c. I also think that max request size should be printed for all drives, not only 48-bit capable.
Quoted 9 lines
Quoted 5 lines
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Then it just puts a taskfile request on the request queue and lets it reach the drive, nicely syncronized with the other requests. There's no need to toggle any special bits for that.
Yes, but patch subtly breakes taskfile :-).
Taskfile ioctl uses do_rw_taskfile() or flagged_taskfile(). Patch replaces drive->adrressing checks by task->addressing, but ide_taskfile_ioctl() doesn't know about it so task->addressing will be always equal 0.
You should add checking for 48-bit commands and setting task->addressing to 1 if neccessary to ide_taskfile_ioctl().
Also changes for pdc202xx_old.c are wrong, we should check for task->addressing not rq_lba48(rq) as taskfile requests also use this codepath.
Patch also misses updates for many uses of drive->addressing (in ide.c, ide-io.c, icside.c, ide-tcq.c and even in ide-taskfile.c).
Quoted 4 lines
Quoted 2 lines
Only after it has verified that 48-bit addressing does work should it set the global flag.
Sounds fine.
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
--
Bartlomiej
Quoted 2 lines
--
Jens Axboe
Quoted 16 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Jens Axboe
Quoted 12 lines · Linus Torvalds
Quoted 7 lines · Jens Axboe
Quoted 4 lines
And testing. In particular, you might want to test whether a device properly supports 48-bit addressing, either from the kernel or from user programs.
For that, a forced 48-bit
hwif->addressinginherited by drives will suffice. And I agree, we should have that.No no no.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
You definitely do NOT want to set "
hwif->addressing" to 1 before you've tested whether it even _works_.Well duh, of course not. Whether a given request is executed in 48-bit or not is a check that _includes_ drive capabilities too of course.
Yeah, we test drive capabilities properly in
idedisk_setup(), but Linus is right speaking about _hwif_ capabilities.
That's a probe time item, even before drives are set up.
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.
Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
Quoted 2 lines
I also think that max request size should be printed for all drives, not only 48-bit capable.
Fine.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Then it just puts a taskfile request on the request queue and lets it reach the drive, nicely syncronized with the other requests. There's no need to toggle any special bits for that.
Quoted 6 lines
Yes, but patch subtly breakes taskfile :-).
Taskfile ioctl uses
do_rw_taskfile()orflagged_taskfile(). Patch replacesdrive->adrressingchecks bytask->addressing, butide_taskfile_ioctl()doesn't know about it sotask->addressingwill be always equal 0.
Uh yes, that is wrong!
Quoted 2 lines
You should add checking for 48-bit commands and setting
task->addressingto 1 if neccessary toide_taskfile_ioctl().
Right
Quoted 3 lines
Also changes for
pdc202xx_old.care wrong, we should check fortask->addressingnotrq_lba48(rq)as taskfile requests also use this codepath.
Ok
Quoted 2 lines
Patch also misses updates for many uses of
drive->addressing(inide.c,ide-io.c,icside.c,ide-tcq.cand even inide-taskfile.c).
Hmm bad grep, weird.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Only after it has verified that 48-bit addressing does work should it set the global flag.
Sounds fine.
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
Agree, I'll update the patch to suit your concerns tomorrow.
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Then it just puts a taskfile request on the request queue and lets it reach the drive, nicely syncronized with the other requests. There's no need to toggle any special bits for that.
Quoted 8 lines
Quoted 6 lines
Yes, but patch subtly breakes taskfile :-).
Taskfile ioctl uses
do_rw_taskfile()orflagged_taskfile(). Patch replacesdrive->adrressingchecks bytask->addressing, butide_taskfile_ioctl()doesn't know about it sotask->addressingwill be always equal 0.Uh yes, that is wrong!
Alright, as per comment this is forced to the drive->addressing now.
Quoted 5 lines
Quoted 3 lines
Also changes for
pdc202xx_old.care wrong, we should check fortask->addressingnotrq_lba48(rq)as taskfile requests also use this codepath.Ok
Looked over this part, and there is no (guarenteed) taskfile associated with the request. So care to expand on this point? As far as I can see, my current code is correct.
Quoted 4 lines
Quoted 2 lines
Patch also misses updates for many uses of
drive->addressing(inide.c,ide-io.c,icside.c,ide-tcq.cand even inide-taskfile.c).Hmm bad grep, weird.
ide.c: ide_dump_status(). this is an ugly one, but to me it already looks correct. we are not throwing away any info by not reading the high bits.
ide-io.c, ide-tcq.c, icside.c: indeed, missed these.
Quoted 4 lines
Quoted 2 lines
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
Agree, I'll update the patch to suit your concerns tomorrow.
Apart from the pdc202xx_old part, I think I've addressed all of your concerns in this patch.
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1119 -> 1.1120
# drivers/ide/ide-taskfile.c 1.16 -> 1.17
# drivers/ide/ppc/pmac.c 1.10 -> 1.11
# include/linux/ide.h 1.48 -> 1.49
# drivers/ide/setup-pci.c 1.13 -> 1.14
# drivers/ide/ide-disk.c 1.40 -> 1.41
# drivers/ide/arm/icside.c 1.6 -> 1.7
# drivers/ide/ide-dma.c 1.13 -> 1.14
# drivers/ide/ide-io.c 1.8 -> 1.9
# drivers/ide/pci/pdc202xx_old.c 1.13 -> 1.14
# drivers/ide/ide-tcq.c 1.2 -> 1.3
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/08 [email protected] 1.1120
# Proper 48-bit lba support
# --------------------------------------------
#
drivers/ide/arm/icside.c +2 -2
@@ -657,7 +657,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (rq_lba48 (rq)) { cmd =WIN_READDMA_EXT ; }else { cmd =WIN_READDMA ;
@@ -698,7 +698,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (rq_lba48 (rq)) { cmd =WIN_WRITEDMA_EXT ; }else { cmd =WIN_WRITEDMA ;
drivers/ide/ide-disk.c +25 -9
@@ -358,7 +358,7 @@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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;ata_nsector_t nsectors;
@@ -383,7 +383,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)) {
@@ -593,7 +593,7 @@return ide_started; } -if (drive->addressing ==1 )/* 48-bit LBA */ +if (rq_lba48 (rq))/* 48-bit LBA */ 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);
@@ -602,9 +602,10 @@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 lba48bit =rq_lba48 (rq); +int cmd =rq_data_dir (rq);if ((cmd ==READ ) && drive->using_tcq)return lba48bit ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;
@@ -631,7 +632,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);
@@ -663,6 +664,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);
@@ -673,7 +675,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;
@@ -701,6 +703,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);
@@ -717,7 +720,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;
@@ -753,6 +756,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);
@@ -1479,7 +1483,7 @@static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +return probe_lba_addressing (drive, arg); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1565,6 +1569,18 @@ } (void )probe_lba_addressing (drive,1 ); + +if (drive->addressing ==1 ) { +ide_hwif_t *hwif =HWIF (drive); +int max_s =2048 ; + +if (max_s > hwif->rqsize) + max_s = hwif->rqsize; + +blk_queue_max_sectors (&drive->queue, max_s); + } + +printk ("%s: max request size: %dKiB\n" , drive->name, drive->queue.max_sectors /2 );/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-dma.c +2 -2
@@ -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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -685,7 +685,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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
drivers/ide/ide-io.c +1 -1
@@ -205,7 +205,7 @@ args->tfRegister[IDE_SELECT_OFFSET ] = hwif->INB (IDE_SELECT_REG ); args->tfRegister[IDE_STATUS_OFFSET ] = stat; -if (drive->addressing ==1 ) { +if (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 );
drivers/ide/ide-taskfile.c +12 -5
@@ -138,7 +138,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 );
@@ -151,7 +151,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 );
@@ -241,7 +241,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 );
@@ -1272,6 +1272,13 @@ args.data_phase = req_task->data_phase; args.command_type = req_task->req_cmd; +/* +* this forces 48-bit commands if the drive is configured to do so. +* it would also be possible to lookup the command type based on the +* opcode, but this is way simpler. +*/ + args.addressing = drive->addressing; +#ifdef CONFIG_IDE_TASK_IOCTL_DEBUG DTF("%s: ide_ioctl_cmd %s: ide_task_cmd %s\n", drive->name,
@@ -1611,13 +1618,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 ); }
drivers/ide/ide-tcq.c +6 -6
@@ -664,20 +664,20 @@ide_startstop_t __ide_dma_queued_read (ide_drive_t *drive) { -u8 command =WIN_READDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_READDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }ide_startstop_t __ide_dma_queued_write (ide_drive_t *drive) { -u8 command =WIN_WRITEDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_WRITEDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_QUEUED_EXT :WIN_WRITEDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }
drivers/ide/pci/pdc202xx_old.c +6 -3
@@ -535,8 +535,9 @@static int pdc202xx_old_ide_dma_begin (ide_drive_t *drive) { -if (drive->addressing ==1 ) { -struct request *rq =HWGROUP (drive)->rq; +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// struct pci_dev *dev = hwif->pci_dev; // unsgned long high_16 = pci_resource_start(dev, 4);
@@ -557,7 +558,9 @@static int pdc202xx_old_ide_dma_end (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4); unsigned long high_16 = hwif->dma_master;
drivers/ide/ppc/pmac.c +2 -4
@@ -1240,7 +1240,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1272,7 +1271,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_READDMA_EXT :WIN_READDMA ; + command =rq_lba48 (rq) ?WIN_READDMA_EXT :WIN_READDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
@@ -1293,7 +1292,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1325,7 +1323,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
drivers/ide/setup-pci.c +11 -0
@@ -659,6 +659,17 @@ d->init_hwif (hwif);/* +* if the driver didn't specify a max request size, set +* to defaults +*/ +if (!hwif->rqsize) { +if (hwif->addressing) + hwif->rqsize =255 ; +else + hwif->rqsize =65535 ; + } + +/* * This is in the wrong place. The driver may * do set up based on the autotune value and this * will then trash it. Torben please move it and
include/linux/ide.h +7 -0
@@ -846,6 +846,12 @@bio_kunmap_irq (buffer, flags); } +/* +* must be addressed with 48-bit lba +*/ +#define rq_lba48 (rq) \ + (((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256) +#define IDE_CHIPSET_PCI_MASK \ ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))#define IDE_CHIPSET_IS_PCI (c) ((IDE_CHIPSET_PCI_MASK >> (c)) & 1)
@@ -1387,6 +1393,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 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.
Quoted 6 lines
Quoted 3 lines
Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this
No, ide_init_queue() sets it to 256, so I want 65536 too. Note that it should be done after setting queue max sectors to 256, because not only ide-disk depends on this code:
max_sectors = 256; (...) /* * Added "< max_sectors" check for safety if it will * be called again later with rq->size = 65536. * I don't believe it ever is. */ if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors); if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;
Quoted 27 lines
is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Imagine something like "hdparm" - other things are already in progress, the system is up, and IDE commands are potentially executing concurrently. What something like that wants to do is to send one request out to check whether 48-bit addressing works, but it absolutely does NOT want to set some interface-global flag that affects other commands.
Then it just puts a taskfile request on the request queue and lets it reach the drive, nicely syncronized with the other requests. There's no need to toggle any special bits for that.
Quoted 8 lines
Quoted 6 lines
Yes, but patch subtly breakes taskfile :-).
Taskfile ioctl uses
do_rw_taskfile()orflagged_taskfile(). Patch replacesdrive->adrressingchecks bytask->addressing, butide_taskfile_ioctl()doesn't know about it sotask->addressingwill be always equal 0.Uh yes, that is wrong!
Alright, as per comment this is forced to the
drive->addressingnow.Quoted 4 lines
Quoted 3 lines
Also changes for
pdc202xx_old.care wrong, we should check fortask->addressingnotrq_lba48(rq)as taskfile requests also use this codepath.
Quoted 5 lines
Quoted 1 line
Ok
Looked over this part, and there is no (guarenteed) taskfile associated with the request. So care to expand on this point? As far as I can see, my current code is correct.
Yeah, I forgot that Linus' tree IDE is not based on taskfile IO yet. But anyway there are REQ_DRIVE_TASKFILE requests using this codepatch, so you should also check rq->flags & REQ_DRIVE_TASKFILE
and task->addreesing.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Patch also misses updates for many uses of
drive->addressing(inide.c,ide-io.c,icside.c,ide-tcq.cand even inide-taskfile.c).Hmm bad grep, weird.
ide.c:ide_dump_status(). this is an ugly one, but to me it already looks correct. we are not throwing away any info by not reading the high bits.
It should check for rq_lba48(rq) || task->addressing.
After taskfile IO switch it will be checking for task->addressing only.
Quoted 9 lines
ide-io.c,ide-tcq.c,icside.c: indeed, missed these.Quoted 4 lines
Quoted 2 lines
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
Agree, I'll update the patch to suit your concerns tomorrow.
Apart from the
pdc202xx_oldpart, I think I've addressed all of your concerns in this patch.
Nope ;-), please move setting of hwif->rqsize from setup-pci.c to where it belongs, ide_init_queue() in ide-probe.c.
--
Bartlomiej
Quoted 8 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this
No,
ide_init_queue()sets it to 256, so I want 65536 too.
Alright, I don't care enough about that 1 sector to argue.
Quoted 17 lines
Note that it should be done after setting queue max sectors to 256, because not only ide-disk depends on this code:
max_sectors = 256; (...) /* * Added "< max_sectors" check for safety if it will * be called again later with rq->size = 65536. * I don't believe it ever is. */ if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors); if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;
You have the logic reversed here, the hwif and drive addressing are reversed. Yeah, it's convoluted, dunno who thought that logic up...
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Patch also misses updates for many uses of
drive->addressing(inide.c,ide-io.c,icside.c,ide-tcq.cand even inide-taskfile.c).Hmm bad grep, weird.
ide.c:ide_dump_status(). this is an ugly one, but to me it already looks correct. we are not throwing away any info by not reading the high bits.It should check for
rq_lba48(rq)||task->addressing.
After taskfile IO switch it will be checking fortask->addressingonly.
This is really ugly, but it has to look like that until you make the global taskfile changes...
Quoted 5 lines
Quoted 5 lines
ide-io.c,ide-tcq.c,icside.c: indeed, missed these.Quoted 3 lines
Quoted 2 lines
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Agree, I'll update the patch to suit your concerns tomorrow.
Apart from the
pdc202xx_oldpart, I think I've addressed all of your concerns in this patch.Nope ;-), please move setting of
hwif->rqsizefromsetup-pci.cto where it belongs,ide_init_queue()inide-probe.c.
So how's this?
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1119 -> 1.1121
# drivers/ide/ide-taskfile.c 1.16 -> 1.17
# drivers/ide/ppc/pmac.c 1.10 -> 1.11
# drivers/ide/ide.c 1.59 -> 1.60
# include/linux/ide.h 1.48 -> 1.49
# drivers/ide/setup-pci.c 1.13 -> 1.15
# drivers/ide/ide-disk.c 1.40 -> 1.41
# drivers/ide/arm/icside.c 1.6 -> 1.7
# drivers/ide/ide-dma.c 1.13 -> 1.14
# drivers/ide/ide-probe.c 1.41 -> 1.42
# drivers/ide/ide-io.c 1.8 -> 1.9
# drivers/ide/pci/pdc202xx_old.c 1.13 -> 1.14
# drivers/ide/ide-tcq.c 1.2 -> 1.3
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/08 [email protected] 1.1120
# Proper 48-bit lba support
# --------------------------------------------
# 03/05/08 [email protected] 1.1121
# IDE 48-bit adjustments
# --------------------------------------------
#
drivers/ide/arm/icside.c +2 -2
@@ -657,7 +657,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (rq_lba48 (rq)) { cmd =WIN_READDMA_EXT ; }else { cmd =WIN_READDMA ;
@@ -698,7 +698,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (rq_lba48 (rq)) { cmd =WIN_WRITEDMA_EXT ; }else { cmd =WIN_WRITEDMA ;
drivers/ide/ide-disk.c +25 -9
@@ -358,7 +358,7 @@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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;ata_nsector_t nsectors;
@@ -383,7 +383,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)) {
@@ -593,7 +593,7 @@return ide_started; } -if (drive->addressing ==1 )/* 48-bit LBA */ +if (rq_lba48 (rq))/* 48-bit LBA */ 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);
@@ -602,9 +602,10 @@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 lba48bit =rq_lba48 (rq); +int cmd =rq_data_dir (rq);if ((cmd ==READ ) && drive->using_tcq)return lba48bit ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;
@@ -631,7 +632,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);
@@ -663,6 +664,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);
@@ -673,7 +675,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;
@@ -701,6 +703,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);
@@ -717,7 +720,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;
@@ -753,6 +756,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);
@@ -1479,7 +1483,7 @@static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +return probe_lba_addressing (drive, arg); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1565,6 +1569,18 @@ } (void )probe_lba_addressing (drive,1 ); + +if (drive->addressing ==1 ) { +ide_hwif_t *hwif =HWIF (drive); +int max_s =2048 ; + +if (max_s > hwif->rqsize) + max_s = hwif->rqsize; + +blk_queue_max_sectors (&drive->queue, max_s); + } + +printk ("%s: max request size: %dKiB\n" , drive->name, drive->queue.max_sectors /2 );/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-dma.c +2 -2
@@ -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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -685,7 +685,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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
drivers/ide/ide-io.c +1 -1
@@ -205,7 +205,7 @@ args->tfRegister[IDE_SELECT_OFFSET ] = hwif->INB (IDE_SELECT_REG ); args->tfRegister[IDE_STATUS_OFFSET ] = stat; -if (drive->addressing ==1 ) { +if (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 );
drivers/ide/ide-probe.c +10 -2
@@ -998,6 +998,7 @@static void ide_init_queue (ide_drive_t *drive) {request_queue_t *q = &drive->queue; +ide_hwif_t *hwif =HWIF (drive);int max_sectors =256 ;/*
@@ -1013,8 +1014,15 @@ drive->queue_setup =1 ;blk_queue_segment_boundary (q,0xffff ); -if (HWIF (drive)->rqsize) - max_sectors =HWIF (drive)->rqsize; +/* +* use rqsize if specified, else set it to defaults for 28-bit or +* 48-bit lba commands +*/ +if (hwif->rqsize) + max_sectors = hwif->rqsize; +else + hwif->rqsize = hwif->addressing ?256 :65536 ; +blk_queue_max_sectors (q, max_sectors);/* IDE DMA can do PRD_ENTRIES number of segments. */
drivers/ide/ide-taskfile.c +12 -5
@@ -138,7 +138,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 );
@@ -151,7 +151,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 );
@@ -241,7 +241,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 );
@@ -1272,6 +1272,13 @@ args.data_phase = req_task->data_phase; args.command_type = req_task->req_cmd; +/* +* this forces 48-bit commands if the drive is configured to do so. +* it would also be possible to lookup the command type based on the +* opcode, but this is way simpler. +*/ + args.addressing = drive->addressing; +#ifdef CONFIG_IDE_TASK_IOCTL_DEBUG DTF("%s: ide_ioctl_cmd %s: ide_task_cmd %s\n", drive->name,
@@ -1611,13 +1618,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 ); }
drivers/ide/ide-tcq.c +6 -6
@@ -664,20 +664,20 @@ide_startstop_t __ide_dma_queued_read (ide_drive_t *drive) { -u8 command =WIN_READDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_READDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }ide_startstop_t __ide_dma_queued_write (ide_drive_t *drive) { -u8 command =WIN_WRITEDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_WRITEDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_QUEUED_EXT :WIN_WRITEDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }
drivers/ide/ide.c +12 -1
@@ -400,9 +400,20 @@if (err &MARK_ERR )printk ("AddrMarkNotFound " );printk ("}" );if ((err & (BBD_ERR |ABRT_ERR )) ==BBD_ERR || (err & (ECC_ERR |ID_ERR |MARK_ERR ))) { +struct request *rq =HWGROUP (drive)->rq; +int lba48 = drive->addressing; + +if (rq) { +if (rq->flags &REQ_DRIVE_TASKFILE ) { +ide_task_t *t = rq->special; + lba48 = t->addressing; + }else + lba48 =rq_lba48 (rq); + } +if ((drive->id->command_set_2 &0x0400 ) && (drive->id->cfs_enable_2 &0x0400 ) && - (drive->addressing ==1 )) { + lba48) {u64 sectors =0 ;u32 high =0 ;u32 low =ide_read_24 (drive);
drivers/ide/pci/pdc202xx_old.c +6 -3
@@ -535,8 +535,9 @@static int pdc202xx_old_ide_dma_begin (ide_drive_t *drive) { -if (drive->addressing ==1 ) { -struct request *rq =HWGROUP (drive)->rq; +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// struct pci_dev *dev = hwif->pci_dev; // unsgned long high_16 = pci_resource_start(dev, 4);
@@ -557,7 +558,9 @@static int pdc202xx_old_ide_dma_end (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4); unsigned long high_16 = hwif->dma_master;
drivers/ide/ppc/pmac.c +2 -4
@@ -1240,7 +1240,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1272,7 +1271,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_READDMA_EXT :WIN_READDMA ; + command =rq_lba48 (rq) ?WIN_READDMA_EXT :WIN_READDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
@@ -1293,7 +1292,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1325,7 +1323,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
include/linux/ide.h +7 -0
@@ -846,6 +846,12 @@bio_kunmap_irq (buffer, flags); } +/* +* must be addressed with 48-bit lba +*/ +#define rq_lba48 (rq) \ + (((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256) +#define IDE_CHIPSET_PCI_MASK \ ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))#define IDE_CHIPSET_IS_PCI (c) ((IDE_CHIPSET_PCI_MASK >> (c)) & 1)
@@ -1387,6 +1393,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 38 lines · Jens Axboe
Quoted 8 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this
No,
ide_init_queue()sets it to 256, so I want 65536 too.Alright, I don't care enough about that 1 sector to argue.
Quoted 17 lines
Note that it should be done after setting queue max sectors to 256, because not only ide-disk depends on this code:
max_sectors = 256; (...) /* * Added "< max_sectors" check for safety if it will * be called again later with rq->size = 65536. * I don't believe it ever is. */ if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors); if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;
Quoted 2 lines
You have the logic reversed here, the hwif and drive addressing are reversed. Yeah, it's convoluted, dunno who thought that logic up...
Not me.
Logic is to prevent some bugs and actually my comment "I don't believe it ever is." is totally wrong.
ide_init_queue() is called for all drives on hwif.
ie. failure scenario:
1st drive 48-bit: !rqsize -> max_sectors = 256, rqsize = 65536 2nd drive 28-bit: rqsize -> max_sectors = 65536 -> doh!
so "< max_sectors" is really needed.
It can look a bit saner:
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256; if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors);
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Patch also misses updates for many uses of
drive->addressing(inide.c,ide-io.c,icside.c,ide-tcq.cand even inide-taskfile.c).Hmm bad grep, weird.
ide.c:ide_dump_status(). this is an ugly one, but to me it already looks correct. we are not throwing away any info by not reading the high bits.
Quoted 5 lines
Quoted 2 lines
It should check for
rq_lba48(rq)||task->addressing.
After taskfile IO switch it will be checking fortask->addressingonly.This is really ugly, but it has to look like that until you make the global taskfile changes...
Yep.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
ide-io.c,ide-tcq.c,icside.c: indeed, missed these.Quoted 4 lines
Quoted 2 lines
Jens, I like the general idea of the patch, but it needs some more work. Linus, please don't apply for now.
Agree, I'll update the patch to suit your concerns tomorrow.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Apart from the
pdc202xx_oldpart, I think I've addressed all of your concerns in this patch.Nope ;-), please move setting of
hwif->rqsizefromsetup-pci.cto where it belongs,ide_init_queue()inide-probe.c.So how's this?
Looks good.
Now test/review it for some time, we don't want any bugs to slip in. :-)
--
Bartlomiej
Quoted 56 lines · Bartlomiej Zolnierkiewicz
Quoted 38 lines · Jens Axboe
Quoted 8 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
Jens you your patch sets
hwif->rqsizeto 65535 insetup-pci.cfor all PCI hwifs which is simply wrong as not all of them supports LBA48. You should check forhwif->addressingand if true set rqsize to 65536 (not 65535) and not in IDE PCI code but inide_init_queue()inide-probe.c.Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Yes you are right, that would be the best way of doing it. As it happens for that patch, it does not hurt or break anything. But it is certainly cleaner, I'll fix that up.
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this
No,
ide_init_queue()sets it to 256, so I want 65536 too.Alright, I don't care enough about that 1 sector to argue.
Quoted 17 lines
Note that it should be done after setting queue max sectors to 256, because not only ide-disk depends on this code:
max_sectors = 256; (...) /* * Added "< max_sectors" check for safety if it will * be called again later with rq->size = 65536. * I don't believe it ever is. */ if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors); if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;Quoted 2 lines
You have the logic reversed here, the hwif and drive addressing are reversed. Yeah, it's convoluted, dunno who thought that logic up...
Not me.
Logic is to prevent some bugs and actually my comment "I don't believe it ever is." is totally wrong.
ide_init_queue()is called for all drives on hwif.ie. failure scenario:
1st drive 48-bit: !rqsize ->max_sectors= 256, rqsize = 65536 2nd drive 28-bit: rqsize ->max_sectors= 65536 -> doh!so "<
max_sectors" is really needed.
Quoted 7 lines
It can look a bit saner:
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256; if (hwif->rqsize < max_sectors) max_sectors = hwif->rqsize; blk_queue_max_sectors(q, max_sectors);
Ugh yeah, that stinks. Your changed version looks better.
Quoted 3 lines
Looks good.
Now test/review it for some time, we don't want any bugs to slip in. :-)
I'll give it a test spin.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Bartlomiej Zolnierkiewicz
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;
btw, you didn't get this right this time either :-)
drive->addressing == 1, 48-bit is okhwif->addressing == 1, 48-bit is _not_ ok
below patch covers the lat change as well, boots and works on my router.
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1119 -> 1.1122
# drivers/ide/ide-taskfile.c 1.16 -> 1.17
# drivers/ide/ppc/pmac.c 1.10 -> 1.11
# drivers/ide/ide.c 1.59 -> 1.60
# include/linux/ide.h 1.48 -> 1.49
# drivers/ide/setup-pci.c 1.13 -> 1.15
# drivers/ide/ide-disk.c 1.40 -> 1.41
# drivers/ide/arm/icside.c 1.6 -> 1.7
# drivers/ide/ide-dma.c 1.13 -> 1.14
# drivers/ide/ide-probe.c 1.41 -> 1.43
# drivers/ide/ide-io.c 1.8 -> 1.9
# drivers/ide/pci/pdc202xx_old.c 1.13 -> 1.14
# drivers/ide/ide-tcq.c 1.2 -> 1.3
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/08 [email protected] 1.1120
# Proper 48-bit lba support
# --------------------------------------------
# 03/05/08 [email protected] 1.1121
# IDE 48-bit adjustments
# --------------------------------------------
# 03/05/08 [email protected] 1.1122
# fix max sectors init logic
# --------------------------------------------
#
diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
--- a/drivers/ide/arm/icside.c Thu May 8 14:32:59 2003
+++ b/drivers/ide/arm/icside.c Thu May 8 14:32:59 2003
@@ -657,7 +657,7 @@
if (rq->flags & REQ_DRIVE_TASKFILE) {
ide_task_t *args = rq->special;
cmd = args->tfRegister[IDE_COMMAND_OFFSET];
- } else if (drive->addressing == 1) {
+ } else if (rq_lba48(rq)) {
cmd = WIN_READDMA_EXT;
} else {
cmd = WIN_READDMA;
@@ -698,7 +698,7 @@
if (rq->flags & REQ_DRIVE_TASKFILE) {
ide_task_t *args = rq->special;
cmd = args->tfRegister[IDE_COMMAND_OFFSET];
- } else if (drive->addressing == 1) {
+ } else if (rq_lba48(rq)) {
cmd = WIN_WRITEDMA_EXT;
} else {
cmd = WIN_WRITEDMA;
diff -Nru a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c Thu May 8 14:32:59 2003
+++ b/drivers/ide/ide-disk.c Thu May 8 14:32:59 2003
@@ -358,7 +358,7 @@
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 = rq_lba48(rq);
task_ioreg_t command = WIN_NOP;
ata_nsector_t nsectors;@@ -383,7 +383,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)) {
@@ -593,7 +593,7 @@
return ide_started;
}
- if (drive->addressing == 1) /* 48-bit LBA */
+ if (rq_lba48(rq)) /* 48-bit LBA */
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);
@@ -602,9 +602,10 @@
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 lba48bit = rq_lba48(rq);
+ int cmd = rq_data_dir(rq);
if ((cmd == READ) && drive->using_tcq)
return lba48bit ? WIN_READDMA_QUEUED_EXT : WIN_READDMA_QUEUED;
@@ -631,7 +632,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);
@@ -663,6 +664,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);
@@ -673,7 +675,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;
@@ -701,6 +703,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);
@@ -717,7 +720,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;
@@ -753,6 +756,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);
@@ -1479,7 +1483,7 @@
static int set_lba_addressing (ide_drive_t *drive, int arg) {
- return (probe_lba_addressing(drive, arg));
+ return probe_lba_addressing(drive, arg);
}
static void idedisk_add_settings(ide_drive_t *drive)
@@ -1565,6 +1569,18 @@
}
(void) probe_lba_addressing(drive, 1);
+
+ if (drive->addressing == 1) {
+ ide_hwif_t *hwif = HWIF(drive);
+ int max_s = 2048;
+
+ if (max_s > hwif->rqsize)
+ max_s = hwif->rqsize;
+
+ blk_queue_max_sectors(&drive->queue, max_s);
+ }
+
+ printk("%s: max request size: %dKiB\n", drive->name, drive->queue.max_sectors / 2);
/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-dma.c +2 -2
@@ -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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -685,7 +685,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 =rq_lba48 (rq);task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
drivers/ide/ide-io.c +1 -1
@@ -205,7 +205,7 @@ args->tfRegister[IDE_SELECT_OFFSET ] = hwif->INB (IDE_SELECT_REG ); args->tfRegister[IDE_STATUS_OFFSET ] = stat; -if (drive->addressing ==1 ) { +if (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 );
drivers/ide/ide-probe.c +5 -2
@@ -998,6 +998,7 @@static void ide_init_queue (ide_drive_t *drive) {request_queue_t *q = &drive->queue; +ide_hwif_t *hwif =HWIF (drive);int max_sectors =256 ;/*
@@ -1013,8 +1014,10 @@ drive->queue_setup =1 ;blk_queue_segment_boundary (q,0xffff ); -if (HWIF (drive)->rqsize) - max_sectors =HWIF (drive)->rqsize; +if (!hwif->rqsize) + hwif->rqsize = hwif->addressing ?256 :65536 ; +if (hwif->rqsize < max_sectors) + max_sectors = hwif->rqsize;blk_queue_max_sectors (q, max_sectors);/* IDE DMA can do PRD_ENTRIES number of segments. */
drivers/ide/ide-taskfile.c +12 -5
@@ -138,7 +138,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 );
@@ -151,7 +151,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 );
@@ -241,7 +241,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 );
@@ -1272,6 +1272,13 @@ args.data_phase = req_task->data_phase; args.command_type = req_task->req_cmd; +/* +* this forces 48-bit commands if the drive is configured to do so. +* it would also be possible to lookup the command type based on the +* opcode, but this is way simpler. +*/ + args.addressing = drive->addressing; +#ifdef CONFIG_IDE_TASK_IOCTL_DEBUG DTF("%s: ide_ioctl_cmd %s: ide_task_cmd %s\n", drive->name,
@@ -1611,13 +1618,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 ); }
drivers/ide/ide-tcq.c +6 -6
@@ -664,20 +664,20 @@ide_startstop_t __ide_dma_queued_read (ide_drive_t *drive) { -u8 command =WIN_READDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_READDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }ide_startstop_t __ide_dma_queued_write (ide_drive_t *drive) { -u8 command =WIN_WRITEDMA_QUEUED ; +struct request *rq = hwgroup->rq; +u8 command; -if (drive->addressing ==1 ) - command =WIN_WRITEDMA_QUEUED_EXT ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_QUEUED_EXT :WIN_WRITEDMA_QUEUED ;return ide_dma_queued_rw (drive, command); }
drivers/ide/ide.c +12 -1
@@ -400,9 +400,20 @@if (err &MARK_ERR )printk ("AddrMarkNotFound " );printk ("}" );if ((err & (BBD_ERR |ABRT_ERR )) ==BBD_ERR || (err & (ECC_ERR |ID_ERR |MARK_ERR ))) { +struct request *rq =HWGROUP (drive)->rq; +int lba48 = drive->addressing; + +if (rq) { +if (rq->flags &REQ_DRIVE_TASKFILE ) { +ide_task_t *t = rq->special; + lba48 = t->addressing; + }else + lba48 =rq_lba48 (rq); + } +if ((drive->id->command_set_2 &0x0400 ) && (drive->id->cfs_enable_2 &0x0400 ) && - (drive->addressing ==1 )) { + lba48) {u64 sectors =0 ;u32 high =0 ;u32 low =ide_read_24 (drive);
drivers/ide/pci/pdc202xx_old.c +6 -3
@@ -535,8 +535,9 @@static int pdc202xx_old_ide_dma_begin (ide_drive_t *drive) { -if (drive->addressing ==1 ) { -struct request *rq =HWGROUP (drive)->rq; +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// struct pci_dev *dev = hwif->pci_dev; // unsgned long high_16 = pci_resource_start(dev, 4);
@@ -557,7 +558,9 @@static int pdc202xx_old_ide_dma_end (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +struct request *rq =HWGROUP (drive)->rq; + +if (rq_lba48 (rq)) {ide_hwif_t *hwif =HWIF (drive);// unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4); unsigned long high_16 = hwif->dma_master;
drivers/ide/ppc/pmac.c +2 -4
@@ -1240,7 +1240,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1272,7 +1271,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_READDMA_EXT :WIN_READDMA ; + command =rq_lba48 (rq) ?WIN_READDMA_EXT :WIN_READDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
@@ -1293,7 +1292,6 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1325,7 +1323,7 @@ command = args->tfRegister[IDE_COMMAND_OFFSET]; }#else - command = (lba48) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ; + command =rq_lba48 (rq) ?WIN_WRITEDMA_EXT :WIN_WRITEDMA ;if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; command = args->tfRegister[IDE_COMMAND_OFFSET ];
include/linux/ide.h +7 -0
@@ -846,6 +846,12 @@bio_kunmap_irq (buffer, flags); } +/* +* must be addressed with 48-bit lba +*/ +#define rq_lba48 (rq) \ + (((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256) +#define IDE_CHIPSET_PCI_MASK \ ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))#define IDE_CHIPSET_IS_PCI (c) ((IDE_CHIPSET_PCI_MASK >> (c)) & 1)
@@ -1387,6 +1393,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 5 lines · Jens Axboe
Quoted 2 lines · Bartlomiej Zolnierkiewicz
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;btw, you didn't get this right this time either :-)
It is right.hwif->addressing means hwif supports 48-bithwif->rqsize means max rq size for _hwif_
Quoted 2 lines
drive->addressing== 1, 48-bit is okhwif->addressing== 1, 48-bit is _not_ ok
And?
Even if !drive->addressing, hwif->addressing can be 1, so hwif->rqsize can be 65536.
Quoted 1 line
below patch covers the lat change as well, boots and works on my router.
Patch still misses pdx202xx_old.c changes :-).
Two new ones:
- rq_lba48(rq) should check for rq->hard_* values
- after some thought, drop _all_ changes to ide_dump_status() (we may hit error when rq->nr_sectors is already < 256)
--
Bartlomiej
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Bartlomiej Zolnierkiewicz
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;btw, you didn't get this right this time either :-)
It is right.
hwif->addressingmeans hwif supports 48-bit
No it doesn't, that's what I keep saying:
static int probe_lba_addressing (ide_drive_t *drive, int arg)
{
drive->addressing = 0;
if (HWIF(drive)->addressing)
return 0;
...so if hwif->addressing != 0, you will never allow 48-bit lba on any units on this hardware interface. So the correct logic is:
hwif->rqsize = hwif->addressing ? 256 : 65536;
as in the patch.
Quoted 1 line
hwif->rqsizemeans max rq size for _hwif_
Correct.
Quoted 5 lines
Quoted 2 lines
drive->addressing== 1, 48-bit is okhwif->addressing== 1, 48-bit is _not_ okAnd?
Even if !drive->addressing,hwif->addressingcan be 1,
If hwif->addressing == 1, drive->addressing will never be anything _but_ 0.
Quoted 1 line
so
hwif->rqsizecan be 65536.
Wrong
Quoted 3 lines
Quoted 1 line
below patch covers the lat change as well, boots and works on my router.
Patch still misses
pdx202xx_old.cchanges :-).
Which?
Quoted 2 lines
Two new ones:
-rq_lba48(rq)should check for rq->hard_* values
Doesn't matter. I actually only thought about dma, in which case it doesn't matter because we never change sector or nr_sectors until after we have called ide_dma_end. For pio, it's no more reliable with hard_* than without. This is all for end_request context of course, at init time it's all the same. Essentially, we _need_ the taskfile changes for this to work. In that case we can limit rq_lba48() to init request time, and set task->addressing and use that from then on.
Quoted 2 lines
- after some thought, drop _all_ changes to
ide_dump_status()(we may hit error whenrq->nr_sectorsis already < 256)
Ditto, cannot be reliable without the taskfile changes.
I won't bother with anything new until the taskfile stuff is in.
Signature · 1 line
Jens Axboe
Quoted 24 lines · Jens Axboe
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Bartlomiej Zolnierkiewicz
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;btw, you didn't get this right this time either :-)
It is right.
hwif->addressingmeans hwif supports 48-bitNo it doesn't, that's what I keep saying:
static int
probe_lba_addressing(ide_drive_t*drive, int arg){ drive->addressing = 0; if (HWIF(drive)->addressing) return 0; ...
Quoted 6 lines
so if
hwif->addressing!= 0, you will never allow 48-bit lba on any units on this hardware interface. So the correct logic is:hwif->rqsize = hwif->addressing ? 256 : 65536;as in the patch.
Yep, you are right, hwif->addressing logic is reversed, what a mess.
Quoted 15 lines
Quoted 1 line
hwif->rqsizemeans max rq size for _hwif_Correct.
Quoted 5 lines
Quoted 2 lines
drive->addressing== 1, 48-bit is okhwif->addressing== 1, 48-bit is _not_ okAnd?
Even if !drive->addressing,hwif->addressingcan be 1,If
hwif->addressing== 1,drive->addressingwill never be anything _but_ 0.Quoted 1 line
so
hwif->rqsizecan be 65536.
Quoted 7 lines
Wrong
Quoted 3 lines
Quoted 1 line
below patch covers the lat change as well, boots and works on my router.
Patch still misses
pdx202xx_old.cchanges :-).Which?
Checking for taskfile requests.
Quoted 10 lines
Quoted 2 lines
Two new ones:
-rq_lba48(rq)should check for rq->hard_* valuesDoesn't matter. I actually only thought about dma, in which case it doesn't matter because we never change sector or
nr_sectorsuntil after we have calledide_dma_end. For pio, it's no more reliable with hard_* than without. This is all forend_requestcontext of course, at init time it's all the same. Essentially, we _need_ the taskfile changes for this to work. In that case we can limitrq_lba48()to init request time, and settask->addressingand use that from then on.
Yes.
Quoted 6 lines
Quoted 2 lines
- after some thought, drop _all_ changes to
ide_dump_status()(we may hit error whenrq->nr_sectorsis already < 256)Ditto, cannot be reliable without the taskfile changes.
I won't bother with anything new until the taskfile stuff is in.
Good decision.
--
Bartlomiej
Quoted 2 lines
--
Jens Axboe
Quoted 26 lines · Bartlomiej Zolnierkiewicz
Quoted 24 lines · Jens Axboe
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Bartlomiej Zolnierkiewicz
if (!hwif->rqsize) hwif->rqsize = hwif->addressing ? 65536 : 256;btw, you didn't get this right this time either :-)
It is right.
hwif->addressingmeans hwif supports 48-bitNo it doesn't, that's what I keep saying:
static int
probe_lba_addressing(ide_drive_t*drive, int arg){ drive->addressing = 0; if (HWIF(drive)->addressing) return 0; ...
Quoted 8 lines
Quoted 6 lines
so if
hwif->addressing!= 0, you will never allow 48-bit lba on any units on this hardware interface. So the correct logic is:hwif->rqsize = hwif->addressing ? 256 : 65536;as in the patch.
Yep, you are right,
hwif->addressinglogic is reversed, what a mess.
Very much so...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Patch still misses
pdx202xx_old.cchanges :-).Which?
Checking for taskfile requests.
Ah ok, same thing I complain about futher down :)
Quoted 5 lines
Quoted 3 lines
Ditto, cannot be reliable without the taskfile changes.
I won't bother with anything new until the taskfile stuff is in.
Good decision.
So what's the time frame on that?
Signature · 1 line
Jens Axboe
Quoted 7 lines · Jens Axboe
Quoted 5 lines
Quoted 3 lines
Ditto, cannot be reliable without the taskfile changes.
I won't bother with anything new until the taskfile stuff is in.
Good decision.
So what's the time frame on that?
And the more important question, how are you creating the taskfile? If it's reachable from end_io context (and not just the submission path), which it must be to solve the problem was are discussing here, then surely it's not from the stack.
BTW, how about a minor compromise. See attached patch, I'd be happy with that for now. It _is_ essentially two seperate issues, right? One is getting good request sizes on 48-bit commands, the other is taking some decent advantage of 48-bit commands.
drivers/ide/ide-disk.c +13 -1
@@ -1479,7 +1483,7 @@static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +return probe_lba_addressing (drive, arg); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1565,6 +1569,18 @@ } (void )probe_lba_addressing (drive,1 ); + +if (drive->addressing ==1 ) { +ide_hwif_t *hwif =HWIF (drive); +int max_s =2048 ; + +if (max_s > hwif->rqsize) + max_s = hwif->rqsize; + +blk_queue_max_sectors (&drive->queue, max_s); + } + +printk ("%s: max request size: %dKiB\n" , drive->name, drive->queue.max_sectors /2 );/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-probe.c +5 -2
@@ -998,6 +998,7 @@static void ide_init_queue (ide_drive_t *drive) {request_queue_t *q = &drive->queue; +ide_hwif_t *hwif =HWIF (drive);int max_sectors =256 ;/*
@@ -1013,8 +1014,10 @@ drive->queue_setup =1 ;blk_queue_segment_boundary (q,0xffff ); -if (HWIF (drive)->rqsize) - max_sectors =HWIF (drive)->rqsize; +if (!hwif->rqsize) + hwif->rqsize = hwif->addressing ?256 :65536 ; +if (hwif->rqsize < max_sectors) + max_sectors = hwif->rqsize;blk_queue_max_sectors (q, max_sectors);/* IDE DMA can do PRD_ENTRIES number of segments. */
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
that for now. It _is_ essentially two seperate issues, right? One is getting good request sizes on 48-bit commands, the other is taking some decent advantage of 48-bit commands.
That came out totally wrong :-)
One issue is getting good request sizes on 48-bit commands (ie taking advantage of 48-bit commands). The other issue, which I left out, is cutting 48-bit lba overhead by not using it when not necessary. The attached patch deals with the former issue, obviously.
Signature · 1 line
Jens Axboe
Quoted 1 line · Bartlomiej Zolnierkiewicz
Yep, you are right,
hwif->addressinglogic is reversed, what a mess.
No the problem is you keep treating it as a binary value. Addressing is a mode. Right now 0 is LBA28/CHS and 1 is LBA48. SATA next generation stuff extends this even further so will I imagine be addressing=2
Quoted 6 lines · Alan Cox
Quoted 1 line · Bartlomiej Zolnierkiewicz
Yep, you are right,
hwif->addressinglogic is reversed, what a mess.No the problem is you keep treating it as a binary value. Addressing is a mode. Right now 0 is LBA28/CHS and 1 is LBA48. SATA next generation stuff extends this even further so will I imagine be addressing=2
You are right but currently it is a binary value.
The same goes for actual usage of drive->addressing and comment in ide.h. --
Bartlomiej
Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines · Alan Cox
Quoted 1 line · Bartlomiej Zolnierkiewicz
Yep, you are right,
hwif->addressinglogic is reversed, what a mess.No the problem is you keep treating it as a binary value. Addressing is a mode. Right now 0 is LBA28/CHS and 1 is LBA48. SATA next generation
For drive->addressing, not hwif.
Quoted 4 lines
Quoted 1 line
stuff extends this even further so will I imagine be addressing=2
You are right but currently it is a binary value.
The same goes for actual usage ofdrive->addressingand comment inide.h.
Maybe a define or two would help here. When you see drive->addressing and hwif->addressing, you assume that they are used identically. That !hwif->addressing means 48-bit is ok, while !drive->addressing means it's not does not help at all.
#define IDE_LBA28 0
#define IDE_LBA48 1
and then change hwif->addressing to be addressed like the drive variant would help a whole lot.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Maybe a define or two would help here. When you see
drive->addressingandhwif->addressing, you assume that they are used identically. That !hwif->addressing means 48-bit is ok, while !drive->addressing means it's not does not help at all.
Why not just change the names? The current setup clearly is confusing, and adding defines doesn't much help. Rename the structure member so that the name says what it is, aka "address_mode", and when renaming it you'd go through the source anyway and change "!addressing" to something more readable like "address_mode == IDE_LBA48" or whatever.
(Anyway, I'll just drop all the 48-bit patches for now, since you've totally confused me about which ones are right and what the bugs are ;)
Linus
Quoted 8 lines · Linus Torvalds
Quoted 5 lines · Jens Axboe
Maybe a define or two would help here. When you see
drive->addressingandhwif->addressing, you assume that they are used identically. That !hwif->addressing means 48-bit is ok, while !drive->addressing means it's not does not help at all.
Quoted 5 lines
Why not just change the names? The current setup clearly is confusing, and adding defines doesn't much help. Rename the structure member so that the name says what it is, aka "
address_mode", and when renaming it you'd go through the source anyway and change "!addressing" to something more readable like "address_mode==IDE_LBA48" or whatever.
Might not be a bad idea, drive->address_mode is a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.
Quoted 2 lines
(Anyway, I'll just drop all the 48-bit patches for now, since you've totally confused me about which ones are right and what the bugs are ;)
I think we can all agree on the last one (attached again, it's short) is ok. The 'only use 48-bit when needed' can wait until Bart gets the taskfile infrastructure in place, until then I'll just have to eat the overhead :)
drivers/ide/ide-disk.c +13 -1
@@ -1479,7 +1483,7 @@static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +return probe_lba_addressing (drive, arg); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1565,6 +1569,18 @@ } (void )probe_lba_addressing (drive,1 ); + +if (drive->addressing ==1 ) { +ide_hwif_t *hwif =HWIF (drive); +int max_s =2048 ; + +if (max_s > hwif->rqsize) + max_s = hwif->rqsize; + +blk_queue_max_sectors (&drive->queue, max_s); + } + +printk ("%s: max request size: %dKiB\n" , drive->name, drive->queue.max_sectors /2 );/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-probe.c +5 -2
@@ -998,6 +998,7 @@static void ide_init_queue (ide_drive_t *drive) {request_queue_t *q = &drive->queue; +ide_hwif_t *hwif =HWIF (drive);int max_sectors =256 ;/*
@@ -1013,8 +1014,10 @@ drive->queue_setup =1 ;blk_queue_segment_boundary (q,0xffff ); -if (HWIF (drive)->rqsize) - max_sectors =HWIF (drive)->rqsize; +if (!hwif->rqsize) + hwif->rqsize = hwif->addressing ?256 :65536 ; +if (hwif->rqsize < max_sectors) + max_sectors = hwif->rqsize;blk_queue_max_sectors (q, max_sectors);/* IDE DMA can do PRD_ENTRIES number of segments. */
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 7 lines · Linus Torvalds
Quoted 5 lines · Jens Axboe
Maybe a define or two would help here. When you see
drive->addressingandhwif->addressing, you assume that they are used identically. That !hwif->addressing means 48-bit is ok, while !drive->addressing means it's not does not help at all.
Quoted 8 lines
Quoted 5 lines
Why not just change the names? The current setup clearly is confusing, and adding defines doesn't much help. Rename the structure member so that the name says what it is, aka "
address_mode", and when renaming it you'd go through the source anyway and change "!addressing" to something more readable like "address_mode==IDE_LBA48" or whatever.Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.
Good idea.
Quoted 2 lines
Quoted 2 lines
(Anyway, I'll just drop all the 48-bit patches for now, since you've totally confused me about which ones are right and what the bugs are ;)
:-)
Quoted 4 lines
I think we can all agree on the last one (attached again, it's short) is ok. The 'only use 48-bit when needed' can wait until Bart gets the taskfile infrastructure in place, until then I'll just have to eat the overhead :)
Okay for me.
btw. Jens, do you have any benchmarks of using 1MiB requests
and/or removing 48-bit overhead?
--
Bartlomiej
Quoted 2 lines · Bartlomiej Zolnierkiewicz
btw. Jens, do you have any benchmarks of using 1MiB requests
and/or removing 48-bit overhead?
I'll try to do some timings on the 48-bit overhead today, should be able to make the analyzer work now...
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.
We don't know if in the future drives will support some random mask of modes. Would
drive->lba48 drive->lba96 drive->..
be safer ?
Quoted 4 lines · Alan Cox
Quoted 2 lines · Jens Axboe
Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.
Quoted 8 lines
We don't know if in the future drives will support some random mask of modes. Would
drive->lba48 drive->lba96 drive->..be safer ?
I had the same thought yesterday, that just because a device does lba89 does not need it supports all of the lower modes. How about just using the drive->address_mode as a supported field of modes?
if (drive->address_mode & IDE_LBA48)
lba48 = 1;
Signature · 1 line
Jens Axboe
RFC PATCH 1/1 Sanitize hwif/drive addressing (was Re: [PATCH] 2.5 ide 48-bit usage)
Quoted 12 lines · Jens Axboe
Quoted 11 lines · Alan Cox
Quoted 2 lines · Jens Axboe
Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.We don't know if in the future drives will support some random mask of modes. Would
drive->lba48 drive->lba96 drive->..
Quoted 8 lines
Quoted 1 line
be safer ?
I had the same thought yesterday, that just because a device does lba89 does not need it supports all of the lower modes. How about just using the
drive->address_modeas a supported field of modes?if (
drive->address_mode&IDE_LBA48)lba48 = 1;
How about something like the attached? Removes ->addressing from both drive and hwif, and adds:
drive->addr_mode: capability mask of addressing modes the drive supports hwif->na_addr_mode: negated capability mask
Patch isn't tested, so this is just a RFC. If we agree on the concept, I can finalize it.
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1083 -> 1.1084
# drivers/ide/ide-taskfile.c 1.16 -> 1.17
# drivers/ide/ppc/pmac.c 1.10 -> 1.11
# drivers/ide/pci/trm290.c 1.11 -> 1.12
# drivers/ide/ide.c 1.59 -> 1.60
# drivers/ide/pci/alim15x3.c 1.11 -> 1.12
# drivers/ide/pci/siimage.c 1.11 -> 1.12
# drivers/ide/ide-disk.c 1.40 -> 1.41
# drivers/ide/arm/icside.c 1.6 -> 1.7
# drivers/ide/legacy/pdc4030.c 1.8 -> 1.9
# drivers/ide/ide-dma.c 1.13 -> 1.14
# drivers/ide/ide-io.c 1.8 -> 1.9
# drivers/ide/pci/pdc202xx_old.c 1.13 -> 1.14
# drivers/ide/ide-tcq.c 1.2 -> 1.3
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/09 [email protected] 1.1084
# Sanitize hwif and drive addressing fields
# --------------------------------------------
#
drivers/ide/arm/icside.c +2 -2
@@ -657,7 +657,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (drive->addr_mode &IDE_ADDR_48BIT ) { cmd =WIN_READDMA_EXT ; }else { cmd =WIN_READDMA ;
@@ -698,7 +698,7 @@if (rq->flags &REQ_DRIVE_TASKFILE ) {ide_task_t *args = rq->special; cmd = args->tfRegister[IDE_COMMAND_OFFSET ]; - }else if (drive->addressing ==1 ) { + }else if (drive->addr_mode &IDE_ADDR_48BIT ) { cmd =WIN_WRITEDMA_EXT ; }else { cmd =WIN_WRITEDMA ;
drivers/ide/ide-disk.c +24 -12
@@ -358,7 +358,7 @@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 = drive->addr_mode &IDE_ADDR_48BIT ;task_ioreg_t command =WIN_NOP ;ata_nsector_t nsectors;
@@ -383,7 +383,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)) {
@@ -593,7 +593,7 @@return ide_started; } -if (drive->addressing ==1 )/* 48-bit LBA */ +if (drive->addr_mode &IDE_ADDR_48BIT )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);
@@ -604,7 +604,7 @@static task_ioreg_t get_command (ide_drive_t *drive,int cmd) { -int lba48bit = (drive->addressing ==1 ) ?1 :0 ; +int lba48bit = drive->addr_mode &IDE_ADDR_48BIT ;if ((cmd ==READ ) && drive->using_tcq)return lba48bit ?WIN_READDMA_QUEUED_EXT :WIN_READDMA_QUEUED ;
@@ -801,7 +801,7 @@printk ("}" );if ((err & (BBD_ERR |ABRT_ERR )) ==BBD_ERR || (err & (ECC_ERR |ID_ERR |MARK_ERR ))) { -if (drive->addressing ==1 ) { +if (drive->addr_mode &IDE_ADDR_48BIT ) {__u64 sectors =0 ;u32 low =0 , high =0 ; low =ide_read_24 (drive);
@@ -1464,22 +1464,34 @@ }#endif -static int probe_lba_addressing (ide_drive_t *drive,int arg) +static int probe_lba48_addressing (ide_drive_t *drive) { - drive->addressing =0 ; +/* +* 28-bit is default capability +*/ + drive->addr_mode =IDE_ADDR_28BIT ; -if (HWIF (drive)->addressing) +if (HWIF (drive)->na_addr_mode &IDE_ADDR_48BIT )return 0 ;if (!(drive->id->cfs_enable_2 &0x0400 ))return -EIO ; - drive->addressing = arg; + + drive->addr_mode |=IDE_ADDR_48BIT ;return 0 ; } +/* +* only check for 48-bit lba capability, we don't support higher than that +*/ static int set_lba_addressing (ide_drive_t *drive,int arg) { -return (probe_lba_addressing (drive, arg)); +if (!arg) { + drive->addr_mode =IDE_ADDR_28BIT ; +return 0 ; + } + +return probe_lba48_addressing (drive); }static void idedisk_add_settings (ide_drive_t *drive)
@@ -1489,7 +1501,7 @@ide_add_setting (drive,"bios_cyl" ,SETTING_RW ,-1 ,-1 ,TYPE_INT ,0 ,65535 ,1 ,1 , &drive->bios_cyl,NULL );ide_add_setting (drive,"bios_head" ,SETTING_RW ,-1 ,-1 ,TYPE_BYTE ,0 ,255 ,1 ,1 , &drive->bios_head,NULL );ide_add_setting (drive,"bios_sect" ,SETTING_RW ,-1 ,-1 ,TYPE_BYTE ,0 ,63 ,1 ,1 , &drive->bios_sect,NULL ); -ide_add_setting (drive,"address" ,SETTING_RW ,HDIO_GET_ADDRESS ,HDIO_SET_ADDRESS ,TYPE_INTA ,0 ,2 ,1 ,1 , &drive->addressing, set_lba_addressing); +ide_add_setting (drive,"address" ,SETTING_RW ,HDIO_GET_ADDRESS ,HDIO_SET_ADDRESS ,TYPE_INTA ,0 ,2 ,1 ,1 , &drive->addr_mode, set_lba_addressing);ide_add_setting (drive,"bswap" ,SETTING_READ ,-1 ,-1 ,TYPE_BYTE ,0 ,1 ,1 ,1 , &drive->bswap,NULL );ide_add_setting (drive,"multcount" , id ?SETTING_RW :SETTING_READ ,HDIO_GET_MULTCOUNT ,HDIO_SET_MULTCOUNT ,TYPE_BYTE ,0 , id ? id->max_multsect :0 ,1 ,1 , &drive->mult_count, set_multcount);ide_add_setting (drive,"nowerr" ,SETTING_RW ,HDIO_GET_NOWERR ,HDIO_SET_NOWERR ,TYPE_BYTE ,0 ,1 ,1 ,1 , &drive->nowerr, set_nowerr);
@@ -1564,7 +1576,7 @@ } } - (void )probe_lba_addressing (drive,1 ); + (void )probe_lba48_addressing (drive);/* Extract geometry if we did not already have one for the drive */ if (!drive->cyl || !drive->head || !drive->sect) {
drivers/ide/ide-dma.c +2 -2
@@ -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 = drive->addr_mode &IDE_ADDR_48BIT ;task_ioreg_t command =WIN_NOP ;/* try pio */
@@ -685,7 +685,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 = drive->addr_mode &IDE_ADDR_48BIT ;task_ioreg_t command =WIN_NOP ;/* try PIO instead of DMA */
drivers/ide/ide-io.c +1 -1
@@ -205,7 +205,7 @@ args->tfRegister[IDE_SELECT_OFFSET ] = hwif->INB (IDE_SELECT_REG ); args->tfRegister[IDE_STATUS_OFFSET ] = stat; -if (drive->addressing ==1 ) { +if (drive->addr_mode &IDE_ADDR_48BIT ) { 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 );
drivers/ide/ide-taskfile.c +8 -7
@@ -138,7 +138,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 = drive->addr_mode &IDE_ADDR_48BIT ;#ifdef CONFIG_IDE_TASK_IOCTL_DEBUG void debug_taskfile (drive ,task );
@@ -151,7 +151,7 @@ }SELECT_MASK (drive,0 ); -if (drive->addressing ==1 ) { +if (drive->addr_mode &IDE_ADDR_48BIT ) { hwif->OUTB (hobfile->feature,IDE_FEATURE_REG ); hwif->OUTB (hobfile->sector_count,IDE_NSECTOR_REG ); hwif->OUTB (hobfile->sector_number,IDE_SECTOR_REG );
@@ -241,7 +241,7 @@ args->tfRegister[IDE_STATUS_OFFSET ] = stat;if ((drive->id->command_set_2 &0x0400 ) && (drive->id->cfs_enable_2 &0x0400 ) && - (drive->addressing ==1 )) { + (drive->addr_mode &IDE_ADDR_48BIT )) { 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 );
@@ -514,7 +514,7 @@ drive->bad_wstat, WAIT_DRQ)) {printk (KERN_ERR "%s: no DRQ after issuing WRITE%s\n" , drive->name, - drive->addressing ?"_EXT" :"" ); + drive->addr_mode &IDE_ADDR_48BIT ?"_EXT" :"" );return startstop; }/* For Write_sectors we need to stuff the first sector */
@@ -597,7 +597,8 @@ drive->bad_wstat, WAIT_DRQ)) {printk (KERN_ERR "%s: no DRQ after issuing %s\n" , drive->name, - drive->addressing ?"MULTWRITE_EXT" :"MULTWRITE" ); + drive->addr_mode &IDE_ADDR_48BIT ? +"MULTWRITE_EXT" :"MULTWRITE" );return startstop; }#ifdef ALTERNATE_STATE_DIAGRAM_MULTI_OUT
@@ -1611,13 +1612,13 @@*/ if (task->tf_out_flags.all ==0 ) { task->tf_out_flags.all =IDE_TASKFILE_STD_OUT_FLAGS ; -if (drive->addressing ==1 ) +if (drive->addr_mode &IDE_ADDR_48BIT ) 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 (drive->addr_mode &IDE_ADDR_48BIT ) task->tf_in_flags.all |= (IDE_HOB_STD_IN_FLAGS <<8 ); }
drivers/ide/ide-tcq.c +2 -2
@@ -666,7 +666,7 @@ {u8 command =WIN_READDMA_QUEUED ; -if (drive->addressing ==1 ) +if (drive->addr_mode &IDE_ADDR_48BIT ) command =WIN_READDMA_QUEUED_EXT ;return ide_dma_queued_rw (drive, command);
@@ -676,7 +676,7 @@ {u8 command =WIN_WRITEDMA_QUEUED ; -if (drive->addressing ==1 ) +if (drive->addr_mode &IDE_ADDR_48BIT ) command =WIN_WRITEDMA_QUEUED_EXT ;return ide_dma_queued_rw (drive, command);
drivers/ide/ide.c +2 -2
@@ -402,7 +402,7 @@if ((err & (BBD_ERR |ABRT_ERR )) ==BBD_ERR || (err & (ECC_ERR |ID_ERR |MARK_ERR ))) {if ((drive->id->command_set_2 &0x0400 ) && (drive->id->cfs_enable_2 &0x0400 ) && - (drive->addressing ==1 )) { + (drive->addr_mode &IDE_ADDR_48BIT )) {u64 sectors =0 ;u32 high =0 ;u32 low =ide_read_24 (drive);
@@ -811,7 +811,7 @@ hwif->mmio = old_hwif.mmio; hwif->rqsize = old_hwif.rqsize; - hwif->addressing = old_hwif.addressing; + hwif->na_addr_mode = old_hwif.na_addr_mode;#ifndef CONFIG_BLK_DEV_IDECS hwif->irq = old_hwif.irq;#endif /* CONFIG_BLK_DEV_IDECS */
drivers/ide/legacy/pdc4030.c +1 -1
@@ -225,7 +225,7 @@ hwif2->mate = hwif; hwif2->channel =1 ; hwif->rqsize = hwif2->rqsize =127 ; - hwif->addressing = hwif2->addressing =1 ; + hwif->na_addr_mode = hwif2->na_addr_mode =IDE_ADDR_48BIT ; hwif->selectproc = hwif2->selectproc = &promise_selectproc; hwif->serialized = hwif2->serialized =1 ;/* DC4030 hosted drives need their own identify... */
drivers/ide/pci/alim15x3.c +2 -1
@@ -745,7 +745,8 @@ hwif->speedproc = &ali15x3_tune_chipset;/* Don't use LBA48 on ALi devices before rev 0xC5 */ - hwif->addressing = (m5229_revision <=0xC4 ) ?1 :0 ; +if (m5229_revision <=0xC4 ) + hwif->na_addr_mode =IDE_ADDR_48BIT ;if (!hwif->dma_base) { hwif->drives[0 ].autotune =1 ;
drivers/ide/pci/pdc202xx_old.c +6 -4
@@ -535,7 +535,7 @@static int pdc202xx_old_ide_dma_begin (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +if (drive->address_mode &IDE_ADDR_48BIT ) {struct request *rq =HWGROUP (drive)->rq;ide_hwif_t *hwif =HWIF (drive);// struct pci_dev *dev = hwif->pci_dev;
@@ -557,7 +557,7 @@static int pdc202xx_old_ide_dma_end (ide_drive_t *drive) { -if (drive->addressing ==1 ) { +if (drive->address_mode &IDE_ADDR_48BIT ) {ide_hwif_t *hwif =HWIF (drive);// unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4); unsigned long high_16 = hwif->dma_master;
@@ -749,8 +749,10 @@ hwif->tuneproc = &config_chipset_for_pio; hwif->quirkproc = &pdc202xx_quirkproc; -if (hwif->pci_dev->device ==PCI_DEVICE_ID_PROMISE_20265 ) - hwif->addressing = (hwif->channel) ?0 :1 ; +if (hwif->pci_dev->device ==PCI_DEVICE_ID_PROMISE_20265 ) { +if (!hwif->channel) + hwif->na_addr_mode =IDE_ADDR_48BIT ; + }if (hwif->pci_dev->device !=PCI_DEVICE_ID_PROMISE_20246 ) { hwif->busproc = &pdc202xx_tristate;
drivers/ide/pci/siimage.c +1 -1
@@ -724,7 +724,7 @@ }#ifdef SIIMAGE_BUFFERED_TASKFILE - hwif->addressing =1 ; + hwif->na_addr_mode =IDE_ADDR_48BIT ;#endif /* SIIMAGE_BUFFERED_TASKFILE */ hwif->irq = hw.irq; hwif->hwif_data =pci_get_drvdata (hwif->pci_dev);
drivers/ide/pci/trm290.c +1 -1
@@ -309,7 +309,7 @@u8 reg =0 ;struct pci_dev *dev = hwif->pci_dev; - hwif->addressing =1 ; + hwif->na_addr_mode =IDE_ADDR_48BIT ; hwif->chipset = ide_trm290; cfgbase =pci_resource_start (dev,4 );if ((dev->class &5 ) && cfgbase) {
drivers/ide/ppc/pmac.c +2 -2
@@ -1240,7 +1240,7 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ; +u8 lba48 = drive->address_mode &IDE_ADDR_48BIT ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
@@ -1293,7 +1293,7 @@// ide_task_t *args = rq->special; u8 unit = (drive->select.b.unit &0x01 );u8 ata4; -u8 lba48 = (drive->addressing ==1 ) ?1 :0 ; +u8 lba48 = drive->address_mode &IDE_ADDR_48BIT ;task_ioreg_t command =WIN_NOP ;if (pmif ==NULL )
Signature · 1 line
Jens Axboe
Re: [PATCH][RFC] Sanitize hwif/drive addressing (was Re: [PATCH] 2.5 ide 48-bit usage)
Quoted 9 lines · Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 7 lines · Alan Cox
Quoted 2 lines · Jens Axboe
Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.We don't know if in the future drives will support some random mask of modes. Would
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drive->lba48 drive->lba96 drive->..be safer ?
I had the same thought yesterday, that just because a device does lba89 does not need it supports all of the lower modes. How about just using
Actually it does for 48-bit.
Quoted 10 lines
Quoted 4 lines
the
drive->address_modeas a supported field of modes?if (
drive->address_mode&IDE_LBA48)lba48 = 1;How about something like the attached? Removes ->addressing from both drive and hwif, and adds:
drive->addr_mode: capability mask of addressing modes the drive supportshwif->na_addr_mode: negated capability mask
Sounds sane.
--
Bartlomiej
Quoted 2 lines
Patch isn't tested, so this is just a RFC. If we agree on the concept, I can finalize it.
Re: [PATCH][RFC] Sanitize hwif/drive addressing (was Re: [PATCH] 2.5 ide 48-bit usage)
Quoted 16 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Jens Axboe
Quoted 12 lines · Jens Axboe
Quoted 11 lines · Alan Cox
Quoted 2 lines · Jens Axboe
Might not be a bad idea,
drive->address_modeis a heck of a lot more to the point. I'll do a swipe of this tomorrow, if no one beats me to it.We don't know if in the future drives will support some random mask of modes. Would
drive->lba48 drive->lba96 drive->..
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
be safer ?
I had the same thought yesterday, that just because a device does lba89 does not need it supports all of the lower modes. How about just using
Actually it does for 48-bit.
Sure, that's not the example :-)
Somewhere down the line, lba28 might (is it already?) be deprecated, for instance.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
the
drive->address_modeas a supported field of modes?if (
drive->address_mode&IDE_LBA48)lba48 = 1;
Quoted 7 lines
Quoted 5 lines
How about something like the attached? Removes ->addressing from both drive and hwif, and adds:
drive->addr_mode: capability mask of addressing modes the drive supportshwif->na_addr_mode: negated capability maskSounds sane.
Can I commit?
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Actually I changed the LBA28 code to use the last sector a while ago. It has (unsuprisingly) caused zero problems because other OS's also generate such requests.
Quoted 8 lines · Alan Cox
Quoted 3 lines · Jens Axboe
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Actually I changed the LBA28 code to use the last sector a while ago. It has (unsuprisingly) caused zero problems because other OS's also generate such requests.
That's great, if you remember that was my requirement for usage of the last sector, that the Other OS used it. If it does, it can't be buggy.
Signature · 1 line
Jens Axboe
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Alan Cox
Quoted 3 lines · Jens Axboe
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Quoted 6 lines
Quoted 3 lines
Actually I changed the LBA28 code to use the last sector a while ago. It has (unsuprisingly) caused zero problems because other OS's also generate such requests.
That's great, if you remember that was my requirement for usage of the last sector, that the Other OS used it. If it does, it can't be buggy.
Yes, there is documentation in ntfs-tools about the kernel not being able to address the last sector like the Other OS does, and recommending to run chkdsk immediately after creating a new ntfs volume under Linux...
Quoted 11 lines · Mike Fedyk
Quoted 10 lines · Jens Axboe
Quoted 8 lines · Alan Cox
Quoted 3 lines · Jens Axboe
That part is added, I still kept it at 65535 though akin to how we don't use that last sector in 28-bit commands either. For 48-bit commands this is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
Actually I changed the LBA28 code to use the last sector a while ago. It has (unsuprisingly) caused zero problems because other OS's also generate such requests.
Quoted 6 lines
Quoted 2 lines
That's great, if you remember that was my requirement for usage of the last sector, that the Other OS used it. If it does, it can't be buggy.
Yes, there is documentation in ntfs-tools about the kernel not being able to address the last sector like the Other OS does, and recommending to run chkdsk immediately after creating a new ntfs volume under Linux...
Completely different thing :-)
In my mail, 'the last sector' means issuing ata commands with 255 or 256 (the last one) sectors. 256 is special because it requires 0 for number of sectors, since it's a byte value.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
Wild guess: You can use larger transfer sizes with the 48 bit interface, even when adressing the lower 28 bit space?
This might be a win for applications that stream large contigous blocks from/to a HD (Video, Audio...)
Regards
Henning
Signature · 5 lines
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH [email protected] +49 9131 50 654 0 http://www.intermeta.de/ Java, perl, Solaris, Linux, xSP Consulting, Web Services freelance consultant -- Jakarta Turbine Development -- hero for hire
Followup to: <[email protected]> By author: "Henning P. Schmiedehausen" <[email protected]> In newsgroup: linux.dev.kernel
Quoted 12 lines
Quoted 2 lines · Jens Axboe
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
Wild guess: You can use larger transfer sizes with the 48 bit interface, even when adressing the lower 28 bit space?
This might be a win for applications that stream large contigous blocks from/to a HD (Video, Audio...)
Right, Jens basically does something like:
use_48_bits := (address+length > 2^28) || (length >= 2^16)
-hpa
Signature · 3 lines
<[email protected]> at work, <[email protected]> in private! "Unix gives you enough rope to shoot yourself in the foot." Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64
Quoted 7 lines · Henning P. Schmiedehausen
Quoted 2 lines · Jens Axboe
I dunno what the purpose of that would be exactly, I guess to cater to some hardware odditites?
Wild guess: You can use larger transfer sizes with the 48 bit interface, even when adressing the lower 28 bit space?
The ide-disk logic Jens did already switches to LBA48 for large requests as well as requests high up on the disk
Quoted 3 lines · Linus Torvalds
At least if I read the patch correctly, theer's no way for upper layers to say "I want 48-bit addressing" - it's just turned on automatically for high sectors (or big transfers).
You read it incorrectly. The lower layers don't know about the issue at all. The disk layer does mapping (conceptually like READ6/READ10/READ16 in SCSI)
Raw I/O and other drivers can still issue CHS LBA28 and LBA48 taskfiles.
Quoted 2 lines
Well, you can mark the drive itself as wanting 48-bit transfers, but you can't do it on a per-request basis.
Its per request at the low level.
Quoted 9 lines · Alan Cox
Quoted 3 lines · Linus Torvalds
At least if I read the patch correctly, theer's no way for upper layers to say "I want 48-bit addressing" - it's just turned on automatically for high sectors (or big transfers).
You read it incorrectly. The lower layers don't know about the issue at all. The disk layer does mapping (conceptually like READ6/READ10/READ16 in SCSI)
Quoted 6 lines
Raw I/O and other drivers can still issue CHS LBA28 and LBA48 taskfiles.
Quoted 2 lines
Well, you can mark the drive itself as wanting 48-bit transfers, but you can't do it on a per-request basis.
Its per request at the low level.
Thanks Alan, that is exactly right of course.
Linus has one point (I'll give him that), in that it might be beneficial to be able to say "48-bit always on" for fs requests. Ie exactly what ide-disk currently does.
Signature · 1 line
Jens Axboe