Re: [PATCH] 2.5 ide 48-bit usage

[Jens Axboe <axboe@suse.de>]

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