[PATCH] 2.5 ide 48-bit usage

[PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

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	axboe@smithers.home.kernel.dk	1.1092
# Proper 48-bit lba support
# --------------------------------------------
#
diff -Nru a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/ide-disk.c	Wed May  7 10:48:37 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,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) {
diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
--- a/drivers/ide/ide-dma.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/ide-dma.c	Wed May  7 10:48:37 2003
@@ -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 */
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/ide-taskfile.c	Wed May  7 10:48:37 2003
@@ -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);
         }
 
diff -Nru a/drivers/ide/pci/pdc202xx_old.c b/drivers/ide/pci/pdc202xx_old.c
--- a/drivers/ide/pci/pdc202xx_old.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/pci/pdc202xx_old.c	Wed May  7 10:48:37 2003
@@ -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;
diff -Nru a/drivers/ide/ppc/pmac.c b/drivers/ide/ppc/pmac.c
--- a/drivers/ide/ppc/pmac.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/ppc/pmac.c	Wed May  7 10:48:37 2003
@@ -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];
diff -Nru a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
--- a/drivers/ide/setup-pci.c	Wed May  7 10:48:37 2003
+++ b/drivers/ide/setup-pci.c	Wed May  7 10:48:37 2003
@@ -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
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	Wed May  7 10:48:37 2003
+++ b/include/linux/ide.h	Wed May  7 10:48:37 2003
@@ -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;

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Linus Torvalds]

On Wed, 7 May 2003, Jens Axboe wrote:
> 
> 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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Linus Torvalds wrote:
> 
> On Wed, 7 May 2003, Jens Axboe wrote:
> > 
> > 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.

> 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?

> That way something like a SG_IO interface 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.

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

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Linus Torvalds]

On Wed, 7 May 2003, Jens Axboe wrote:
> 
> 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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Linus Torvalds wrote:
> 
> On Wed, 7 May 2003, Jens Axboe wrote:
> > 
> > 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?!

> 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Linus Torvalds]

On Wed, 7 May 2003, Jens Axboe wrote:
> > 
> > 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.

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.

> 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.

That may well be sufficient. 

		Linus

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Linus Torvalds wrote:
> 
> On Wed, 7 May 2003, Jens Axboe wrote:
> > > 
> > > 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.
> 
> 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.

> 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.

> Only after it has verified that 48-bit addressing does work should it set 
> the global flag.

Sounds fine.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Wed, 7 May 2003, Jens Axboe wrote:

> On Wed, May 07 2003, Linus Torvalds wrote:
> >
> > On Wed, 7 May 2003, Jens Axboe wrote:
> > > >
> > > > 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.
> >
> > 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.

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

> > 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

> --
> Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Wed, 7 May 2003, Jens Axboe wrote:
> 
> > On Wed, May 07 2003, Linus Torvalds wrote:
> > >
> > > On Wed, 7 May 2003, Jens Axboe wrote:
> > > > >
> > > > > 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.
> > >
> > > 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.

That's a probe time item, even before drives are set up.

> 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.

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.

> I also think that max request size should be printed for all drives,
> not only 48-bit capable.

Fine.

> > > 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.

Uh yes, that is wrong!

> You should add checking for 48-bit commands and setting task->addressing
> to 1 if neccessary to ide_taskfile_ioctl().

Right

> 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.

Ok

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

Hmm bad grep, weird.

> > > 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Jens Axboe wrote:
> > 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.
> 
> 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.

> > > > 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.
> 
> Uh yes, that is wrong!

Alright, as per comment this is forced to the drive->addressing now.

> > 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.
> 
> 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.

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

> > 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	axboe@smithers.home.kernel.dk	1.1120
# Proper 48-bit lba support
# --------------------------------------------
#
diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
--- a/drivers/ide/arm/icside.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/arm/icside.c	Thu May  8 09:55:45 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 09:55:45 2003
+++ b/drivers/ide/ide-disk.c	Thu May  8 09:55:45 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) {
diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
--- a/drivers/ide/ide-dma.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/ide-dma.c	Thu May  8 09:55:45 2003
@@ -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 */
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/ide-io.c	Thu May  8 09:55:45 2003
@@ -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);
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/ide-taskfile.c	Thu May  8 09:55:45 2003
@@ -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);
         }
 
diff -Nru a/drivers/ide/ide-tcq.c b/drivers/ide/ide-tcq.c
--- a/drivers/ide/ide-tcq.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/ide-tcq.c	Thu May  8 09:55:45 2003
@@ -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);
 }
diff -Nru a/drivers/ide/pci/pdc202xx_old.c b/drivers/ide/pci/pdc202xx_old.c
--- a/drivers/ide/pci/pdc202xx_old.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/pci/pdc202xx_old.c	Thu May  8 09:55:45 2003
@@ -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;
diff -Nru a/drivers/ide/ppc/pmac.c b/drivers/ide/ppc/pmac.c
--- a/drivers/ide/ppc/pmac.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/ppc/pmac.c	Thu May  8 09:55:45 2003
@@ -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];
diff -Nru a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c
--- a/drivers/ide/setup-pci.c	Thu May  8 09:55:45 2003
+++ b/drivers/ide/setup-pci.c	Thu May  8 09:55:45 2003
@@ -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
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	Thu May  8 09:55:45 2003
+++ b/include/linux/ide.h	Thu May  8 09:55:45 2003
@@ -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;

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Alan Cox]

On Iau, 2003-05-08 at 08:56, Jens Axboe wrote:
> 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.

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Alan Cox wrote:
> On Iau, 2003-05-08 at 08:56, Jens Axboe wrote:
> > 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Mike Fedyk]

On Thu, May 08, 2003 at 02:01:45PM +0200, Jens Axboe wrote:
> On Thu, May 08 2003, Alan Cox wrote:
> > On Iau, 2003-05-08 at 08:56, Jens Axboe wrote:
> > > 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.

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...

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Mon, May 12 2003, Mike Fedyk wrote:
> On Thu, May 08, 2003 at 02:01:45PM +0200, Jens Axboe wrote:
> > On Thu, May 08 2003, Alan Cox wrote:
> > > On Iau, 2003-05-08 at 08:56, Jens Axboe wrote:
> > > > 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.
> 
> 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Thu, 8 May 2003, Jens Axboe wrote:
> On Wed, May 07 2003, Jens Axboe wrote:
> > > 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.
> >
> > 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;

> is totally irelevant, 32MiB or 32MiB-512b doesn't matter either way.
>
> > > > > 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.
> >
> > Uh yes, that is wrong!
>
> Alright, as per comment this is forced to the drive->addressing now.
>
> > > 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.
> >
> > 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.

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

> ide-io.c, ide-tcq.c, icside.c: indeed, missed these.
>
> > > 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.

Nope ;-), please move setting of hwif->rqsize from setup-pci.c to where
it belongs, ide_init_queue() in ide-probe.c.

--
Bartlomiej

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Thu, 8 May 2003, Jens Axboe wrote:
> > On Wed, May 07 2003, Jens Axboe wrote:
> > > > 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.
> > >
> > > 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.

> 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...

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

This is really ugly, but it has to look like that until you make the
global taskfile changes...

> > ide-io.c, ide-tcq.c, icside.c: indeed, missed these.
> >
> > > > 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.
> 
> Nope ;-), please move setting of hwif->rqsize from setup-pci.c to where
> it belongs, ide_init_queue() in ide-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	axboe@smithers.home.kernel.dk	1.1120
# Proper 48-bit lba support
# --------------------------------------------
# 03/05/08	axboe@smithers.home.kernel.dk	1.1121
# IDE 48-bit adjustments
# --------------------------------------------
#
diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
--- a/drivers/ide/arm/icside.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/arm/icside.c	Thu May  8 13:58:42 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 13:58:42 2003
+++ b/drivers/ide/ide-disk.c	Thu May  8 13:58:42 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) {
diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
--- a/drivers/ide/ide-dma.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide-dma.c	Thu May  8 13:58:42 2003
@@ -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 */
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide-io.c	Thu May  8 13:58:42 2003
@@ -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);
diff -Nru a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
--- a/drivers/ide/ide-probe.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide-probe.c	Thu May  8 13:58:42 2003
@@ -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. */
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide-taskfile.c	Thu May  8 13:58:42 2003
@@ -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);
         }
 
diff -Nru a/drivers/ide/ide-tcq.c b/drivers/ide/ide-tcq.c
--- a/drivers/ide/ide-tcq.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide-tcq.c	Thu May  8 13:58:42 2003
@@ -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);
 }
diff -Nru a/drivers/ide/ide.c b/drivers/ide/ide.c
--- a/drivers/ide/ide.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ide.c	Thu May  8 13:58:42 2003
@@ -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);
diff -Nru a/drivers/ide/pci/pdc202xx_old.c b/drivers/ide/pci/pdc202xx_old.c
--- a/drivers/ide/pci/pdc202xx_old.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/pci/pdc202xx_old.c	Thu May  8 13:58:42 2003
@@ -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;
diff -Nru a/drivers/ide/ppc/pmac.c b/drivers/ide/ppc/pmac.c
--- a/drivers/ide/ppc/pmac.c	Thu May  8 13:58:42 2003
+++ b/drivers/ide/ppc/pmac.c	Thu May  8 13:58:42 2003
@@ -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];
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	Thu May  8 13:58:42 2003
+++ b/include/linux/ide.h	Thu May  8 13:58:42 2003
@@ -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;

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Thu, 8 May 2003, Jens Axboe wrote:

> On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> >
> > On Thu, 8 May 2003, Jens Axboe wrote:
> > > On Wed, May 07 2003, Jens Axboe wrote:
> > > > > 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.
> > > >
> > > > 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.
>
> > 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...

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);

> > > > > 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).
> > > >
> > > > 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.
>
> This is really ugly, but it has to look like that until you make the
> global taskfile changes...

Yep.

> > > ide-io.c, ide-tcq.c, icside.c: indeed, missed these.
> > >
> > > > > 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.
> >
> > Nope ;-), please move setting of hwif->rqsize from setup-pci.c to where
> > it belongs, ide_init_queue() in ide-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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Thu, 8 May 2003, Jens Axboe wrote:
> 
> > On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > On Thu, 8 May 2003, Jens Axboe wrote:
> > > > On Wed, May 07 2003, Jens Axboe wrote:
> > > > > > 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.
> > > > >
> > > > > 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.
> >
> > > 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...
> 
> 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);

Ugh yeah, that stinks. Your changed version looks better.

> 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

n Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 	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 ok
hwif->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	axboe@smithers.home.kernel.dk	1.1120
# Proper 48-bit lba support
# --------------------------------------------
# 03/05/08	axboe@smithers.home.kernel.dk	1.1121
# IDE 48-bit adjustments
# --------------------------------------------
# 03/05/08	axboe@smithers.home.kernel.dk	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) {
diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
--- a/drivers/ide/ide-dma.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-dma.c	Thu May  8 14:32:59 2003
@@ -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 */
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-io.c	Thu May  8 14:32:59 2003
@@ -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);
diff -Nru a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
--- a/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
@@ -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. */
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-taskfile.c	Thu May  8 14:32:59 2003
@@ -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);
         }
 
diff -Nru a/drivers/ide/ide-tcq.c b/drivers/ide/ide-tcq.c
--- a/drivers/ide/ide-tcq.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-tcq.c	Thu May  8 14:32:59 2003
@@ -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);
 }
diff -Nru a/drivers/ide/ide.c b/drivers/ide/ide.c
--- a/drivers/ide/ide.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide.c	Thu May  8 14:32:59 2003
@@ -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);
diff -Nru a/drivers/ide/pci/pdc202xx_old.c b/drivers/ide/pci/pdc202xx_old.c
--- a/drivers/ide/pci/pdc202xx_old.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/pci/pdc202xx_old.c	Thu May  8 14:32:59 2003
@@ -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;
diff -Nru a/drivers/ide/ppc/pmac.c b/drivers/ide/ppc/pmac.c
--- a/drivers/ide/ppc/pmac.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ppc/pmac.c	Thu May  8 14:32:59 2003
@@ -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];
diff -Nru a/include/linux/ide.h b/include/linux/ide.h
--- a/include/linux/ide.h	Thu May  8 14:32:59 2003
+++ b/include/linux/ide.h	Thu May  8 14:32:59 2003
@@ -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;

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Thu, 8 May 2003, Jens Axboe wrote:

> n Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > 	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-bit
hwif->rqsize means max rq size for _hwif_

> drive->addressing == 1, 48-bit is ok
> hwif->addressing == 1, 48-bit is _not_ ok

And?
Even if !drive->addressing, hwif->addressing can be 1,
so hwif->rqsize can be 65536.

> 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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Thu, 8 May 2003, Jens Axboe wrote:
> 
> > n Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > > 	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-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.

> hwif->rqsize means max rq size for _hwif_

Correct.

> > drive->addressing == 1, 48-bit is ok
> > hwif->addressing == 1, 48-bit is _not_ ok
> 
> And?
> Even if !drive->addressing, hwif->addressing can be 1,

If hwif->addressing == 1, drive->addressing will never be anything _but_
0.

> so hwif->rqsize can be 65536.

Wrong

> > below patch covers the lat change as well, boots and works on my router.
> 
> Patch still misses pdx202xx_old.c changes :-).

Which?

> 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.

> - after some thought, drop _all_ changes to ide_dump_status()
>   (we may hit error when rq->nr_sectors is already < 256)

Ditto, cannot be reliable without the taskfile changes.

I won't bother with anything new until the taskfile stuff is in.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Thu, 8 May 2003, Jens Axboe wrote:
> On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> >
> > On Thu, 8 May 2003, Jens Axboe wrote:
> >
> > > n Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > > > 	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-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.

Yep, you are right, hwif->addressing logic is reversed, what a mess.

> > hwif->rqsize means max rq size for _hwif_
>
> Correct.
>
> > > drive->addressing == 1, 48-bit is ok
> > > hwif->addressing == 1, 48-bit is _not_ ok
> >
> > And?
> > Even if !drive->addressing, hwif->addressing can be 1,
>
> If hwif->addressing == 1, drive->addressing will never be anything _but_
> 0.
>
> > so hwif->rqsize can be 65536.
>
> Wrong
>
> > > below patch covers the lat change as well, boots and works on my router.
> >
> > Patch still misses pdx202xx_old.c changes :-).
>
> Which?

Checking for taskfile requests.

> > 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.

Yes.

> > - after some thought, drop _all_ changes to ide_dump_status()
> >   (we may hit error when rq->nr_sectors is 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

> --
> Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Thu, 8 May 2003, Jens Axboe wrote:
> > On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > On Thu, 8 May 2003, Jens Axboe wrote:
> > >
> > > > n Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> > > > > 	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-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.
> 
> Yep, you are right, hwif->addressing logic is reversed, what a mess.

Very much so...

> > > Patch still misses pdx202xx_old.c changes :-).
> >
> > Which?
> 
> Checking for taskfile requests.

Ah ok, same thing I complain about futher down :)

> > 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?

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Jens Axboe wrote:
> > > 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.

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
@@ -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) {
diff -Nru a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
--- a/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
@@ -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. */

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Jens Axboe wrote:
> 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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Alan Cox]

On Iau, 2003-05-08 at 14:35, Bartlomiej Zolnierkiewicz wrote:
> Yep, you are right, hwif->addressing logic 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

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On 8 May 2003, Alan Cox wrote:
> On Iau, 2003-05-08 at 14:35, Bartlomiej Zolnierkiewicz wrote:
> > Yep, you are right, hwif->addressing logic 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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On 8 May 2003, Alan Cox wrote:
> > On Iau, 2003-05-08 at 14:35, Bartlomiej Zolnierkiewicz wrote:
> > > Yep, you are right, hwif->addressing logic 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.

> > 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.

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.

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Linus Torvalds]

On Thu, 8 May 2003, Jens Axboe wrote:
> 
> 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.

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

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Linus Torvalds wrote:
> 
> On Thu, 8 May 2003, Jens Axboe wrote:
> > 
> > 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.
> 
> 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.

> (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 :)

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
@@ -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) {
diff -Nru a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
--- a/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
+++ b/drivers/ide/ide-probe.c	Thu May  8 14:32:59 2003
@@ -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. */

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Bartlomiej Zolnierkiewicz]

On Thu, 8 May 2003, Jens Axboe wrote:
> On Thu, May 08 2003, Linus Torvalds wrote:
> > On Thu, 8 May 2003, Jens Axboe wrote:
> > >
> > > 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.
> >
> > 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.

Good idea.

> > (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 :)

Okay for me.

btw. Jens, do you have any benchmarks of using 1MiB requests
     and/or removing 48-bit overhead?
--
Bartlomiej

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote:
> 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...

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Alan Cox]

On Iau, 2003-05-08 at 17:34, Jens Axboe wrote:
> 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.

We don't know if in the future drives will support some random mask of modes.
Would

	drive->lba48
	drive->lba96
	drive->..

be safer ?

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Thu, May 08 2003, Alan Cox wrote:
> On Iau, 2003-05-08 at 17:34, Jens Axboe wrote:
> > 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.
> 
> 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;

-- 
Jens Axboe

[PATCH][RFC] Sanitize hwif/drive addressing (was Re: [PATCH] 2.5 ide 48-bit usage)

[Jens Axboe]

On Fri, May 09 2003, Jens Axboe wrote:
> On Thu, May 08 2003, Alan Cox wrote:
> > On Iau, 2003-05-08 at 17:34, Jens Axboe wrote:
> > > 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.
> > 
> > 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;

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	axboe@smithers.home.kernel.dk	1.1084
# Sanitize hwif and drive addressing fields
# --------------------------------------------
#
diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
--- a/drivers/ide/arm/icside.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/arm/icside.c	Fri May  9 10:27:36 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 (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;
diff -Nru a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide-disk.c	Fri May  9 10:27:36 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		= 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) {
diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
--- a/drivers/ide/ide-dma.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide-dma.c	Fri May  9 10:27:36 2003
@@ -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 */
diff -Nru a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide-io.c	Fri May  9 10:27:36 2003
@@ -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);
diff -Nru a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide-taskfile.c	Fri May  9 10:27:36 2003
@@ -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);
         }
 
diff -Nru a/drivers/ide/ide-tcq.c b/drivers/ide/ide-tcq.c
--- a/drivers/ide/ide-tcq.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide-tcq.c	Fri May  9 10:27:36 2003
@@ -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);
diff -Nru a/drivers/ide/ide.c b/drivers/ide/ide.c
--- a/drivers/ide/ide.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ide.c	Fri May  9 10:27:36 2003
@@ -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 */
diff -Nru a/drivers/ide/legacy/pdc4030.c b/drivers/ide/legacy/pdc4030.c
--- a/drivers/ide/legacy/pdc4030.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/legacy/pdc4030.c	Fri May  9 10:27:36 2003
@@ -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... */
diff -Nru a/drivers/ide/pci/alim15x3.c b/drivers/ide/pci/alim15x3.c
--- a/drivers/ide/pci/alim15x3.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/pci/alim15x3.c	Fri May  9 10:27:36 2003
@@ -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;
diff -Nru a/drivers/ide/pci/pdc202xx_old.c b/drivers/ide/pci/pdc202xx_old.c
--- a/drivers/ide/pci/pdc202xx_old.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/pci/pdc202xx_old.c	Fri May  9 10:27:36 2003
@@ -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;
diff -Nru a/drivers/ide/pci/siimage.c b/drivers/ide/pci/siimage.c
--- a/drivers/ide/pci/siimage.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/pci/siimage.c	Fri May  9 10:27:36 2003
@@ -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);
diff -Nru a/drivers/ide/pci/trm290.c b/drivers/ide/pci/trm290.c
--- a/drivers/ide/pci/trm290.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/pci/trm290.c	Fri May  9 10:27:36 2003
@@ -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) {
diff -Nru a/drivers/ide/ppc/pmac.c b/drivers/ide/ppc/pmac.c
--- a/drivers/ide/ppc/pmac.c	Fri May  9 10:27:36 2003
+++ b/drivers/ide/ppc/pmac.c	Fri May  9 10:27:36 2003
@@ -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)

-- 
Jens Axboe

Re: [PATCH][RFC] Sanitize hwif/drive addressing (was Re: [PATCH] 2.5 ide 48-bit usage)

[Bartlomiej Zolnierkiewicz]

On Fri, 9 May 2003, Jens Axboe wrote:

> On Fri, May 09 2003, Jens Axboe wrote:
> > On Thu, May 08 2003, Alan Cox wrote:
> > > On Iau, 2003-05-08 at 17:34, Jens Axboe wrote:
> > > > 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.
> > >
> > > 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

Actually it does for 48-bit.

> > the drive->address_mode as 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

Sounds sane.
--
Bartlomiej

> 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)

[Jens Axboe]

On Fri, May 09 2003, Bartlomiej Zolnierkiewicz wrote:
> 
> On Fri, 9 May 2003, Jens Axboe wrote:
> 
> > On Fri, May 09 2003, Jens Axboe wrote:
> > > On Thu, May 08 2003, Alan Cox wrote:
> > > > On Iau, 2003-05-08 at 17:34, Jens Axboe wrote:
> > > > > 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.
> > > >
> > > > 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
> 
> 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.

> > > the drive->address_mode as 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
> 
> Sounds sane.

Can I commit?

-- 
Jens Axboe

Re: [PATCH] 2.5 ide 48-bit usage

[Henning P. Schmiedehausen]

Jens Axboe <axboe@suse.de> writes:

>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


-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen          INTERMETA GmbH
hps@intermeta.de        +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

Re: [PATCH] 2.5 ide 48-bit usage

[Alan Cox]

On Mer, 2003-05-07 at 22:45, Henning P. Schmiedehausen wrote:
> Jens Axboe <axboe@suse.de> writes:
> 
> >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

Re: [PATCH] 2.5 ide 48-bit usage

[H. Peter Anvin]

Followup to:  <b9bupr$jkm$2@tangens.hometree.net>
By author:    "Henning P. Schmiedehausen" <hps@intermeta.de>
In newsgroup: linux.dev.kernel
>
> Jens Axboe <axboe@suse.de> writes:
> 
> >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
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> 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

Re: [PATCH] 2.5 ide 48-bit usage

[Alan Cox]

On Mer, 2003-05-07 at 17:28, Linus Torvalds wrote:
> 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.

> 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.

Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe]

On Wed, May 07 2003, Alan Cox wrote:
> On Mer, 2003-05-07 at 17:28, Linus Torvalds wrote:
> > 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.
> 
> > 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.

-- 
Jens Axboe