libata: make some configurations per-dev

PATCH New Tejun Heo 5 participants 24 messages Feb 12, 2006 – Feb 20, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Tejun Heo

author

2006-02-12 14:33 UTC #0 lore raw

PATCH 1/1 libata: make some configurations per-dev

Hello, Jeff.

This patchset is another prep patchset for configuration reorganization, concentrating on converting several configuration parameters per-dev from being per-host.

This patchset is generated on top of

the current upstream[1]
+ misc preps for configuration reorganization[2]
+ 'don't do EDD handling if ->probe_reset is used' patch[3]

But should also play well with piix related patches[4].

Thanks.

--
tejun

[1] bef4a456b8dc8b3638f4d49a25a89e1467da9483
[2] http://article.gmane.org/gmane.linux.ide/7977
[3] http://article.gmane.org/gmane.linux.ide/7979
[4] http://article.gmane.org/gmane.linux.ide/7967

    http://article.gmane.org/gmane.linux.ide/7968
    http://article.gmane.org/gmane.linux.ide/7969
    http://article.gmane.org/gmane.linux.ide/7970
    http://article.gmane.org/gmane.linux.ide/7971
    http://article.gmane.org/gmane.linux.ide/7972

Tejun Heo

author

2006-02-12 14:33 UTC #1 lore raw

PATCH 1/4 libata: make ata_dev_knobble() per-device

ata_dev_knobble() unconditionally used the first device of the port to determine whether a device is bridged or not. This causes bridge limit to be incorrectly applied or unapplied for hosts with slave devices (e.g. ata_piix).

Signed-off-by Tejun Heo <[email protected]>

---

 drivers/scsi/libata-core.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

f6a90d153c45a62fb4fdd3a3c575e8163182d675

drivers/scsi/libata-core.c +5 -4

index 7a88e91..01d0ca8 100644
@@ -1121,9 +1121,10 @@ err_out:
 }
 
 
-static inline u8 ata_dev_knobble(const struct ata_port *ap)
+static inline u8 ata_dev_knobble(const struct ata_port *ap,
+				 struct ata_device *dev)
 {
-	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device->id)));
+	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
 }
 
 /**

@@ -1137,9 +1138,9 @@ static inline u8 ata_dev_knobble(const s
 void ata_dev_config(struct ata_port *ap, unsigned int i)
 {
 	/* limit bridge transfers to udma5, 200 sectors */
-	if (ata_dev_knobble(ap)) {
+	if (ata_dev_knobble(ap, &ap->device[i])) {
 		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
-			ap->id, ap->device->devno);
+		       ap->id, i);
 		ap->udma_mask &= ATA_UDMA5;
 		ap->host->max_sectors = ATA_MAX_SECTORS;
 		ap->host->hostt->max_sectors = ATA_MAX_SECTORS;

Signature · 1 line

1.1.5

Jeff Garzik

2006-02-12 19:29 UTC #2 lore raw

Re: [PATCH 1/4] libata: make ata_dev_knobble() per-device

Quoted 6 lines · Tejun Heo

ata_dev_knobble() unconditionally used the first device of the port to determine whether a device is bridged or not. This causes bridge limit to be incorrectly applied or unapplied for hosts with slave devices (e.g. ata_piix).
Signed-off-by Tejun Heo <[email protected]>

applied 1-2

Tejun Heo

author

2006-02-12 14:33 UTC #3 lore raw

PATCH 2/4 libata: move cdb_len for host to device

In reply to Tejun Heo

cdb_len is per-device property. Sharing cdb_len on ap results in inaccurate configuration on revalidation and hotplugging. This patch makes cdb_len per-device.

Signed-off-by Tejun Heo <[email protected]>

---

 drivers/scsi/ahci.c        |    3 ++-
 drivers/scsi/libata-core.c |   19 ++++++++++++-------
 drivers/scsi/libata-scsi.c |    4 ++--
 drivers/scsi/sata_sil24.c  |    4 ++--
 include/linux/libata.h     |    2 +-
 5 files changed, 19 insertions(+), 13 deletions(-)

3adaa59ad21ad95f5bad4410b8d4334a3f703185

drivers/scsi/ahci.c +2 -1

index 24a54a5..23caa0c 100644
@@ -617,7 +617,8 @@ static void ahci_qc_prep(struct ata_queu
 	ata_tf_to_fis(&qc->tf, pp->cmd_tbl, 0);
 	if (is_atapi) {
 		memset(pp->cmd_tbl + AHCI_CMD_TBL_CDB, 0, 32);
-		memcpy(pp->cmd_tbl + AHCI_CMD_TBL_CDB, qc->cdb, ap->cdb_len);
+		memcpy(pp->cmd_tbl + AHCI_CMD_TBL_CDB, qc->cdb,
+		       qc->dev->cdb_len);
 	}
 
 	n_elem = 0;

drivers/scsi/libata-core.c +12 -7

index 01d0ca8..c62798c 100644
@@ -931,7 +931,7 @@ static void ata_dev_identify(struct ata_
 	unsigned int using_edd;
 	struct ata_taskfile tf;
 	unsigned int err_mask;
-	int rc;
+	int i, rc;
 
 	if (!ata_dev_present(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",

@@ -1087,7 +1087,7 @@ retry:
 
 		}
 
-		ap->host->max_cmd_len = 16;
+		dev->cdb_len = 16;
 	}
 
 	/* ATAPI-specific feature tests */

@@ -1100,8 +1100,7 @@ retry:
 			printk(KERN_WARNING "ata%u: unsupported CDB len\n", ap->id);
 			goto err_out_nosup;
 		}
-		ap->cdb_len = (unsigned int) rc;
-		ap->host->max_cmd_len = (unsigned char) ap->cdb_len;
+		dev->cdb_len = (unsigned int) rc;
 
 		/* print device info to dmesg */
 		printk(KERN_INFO "ata%u: dev %u ATAPI, max %s\n",

@@ -1109,6 +1108,12 @@ retry:
 		       ata_mode_string(xfer_modes));
 	}
 
+	ap->host->max_cmd_len = 0;
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		ap->host->max_cmd_len = max_t(unsigned int,
+					      ap->host->max_cmd_len,
+					      ap->device[i].cdb_len);
+
 	DPRINTK("EXIT, drv_stat = 0x%x\n", ata_chk_status(ap));
 	return;

@@ -4200,7 +4205,7 @@ static void atapi_packet_task(void *_dat
 
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
-	WARN_ON(ap->cdb_len < 12);
+	WARN_ON(qc->dev->cdb_len < 12);
 
 	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
 	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {

@@ -4214,12 +4219,12 @@ static void atapi_packet_task(void *_dat
 		 */
 		spin_lock_irqsave(&ap->host_set->lock, flags);
 		ap->flags &= ~ATA_FLAG_NOINTR;
-		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
 		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
 			ap->ops->bmdma_start(qc);	/* initiate bmdma */
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	} else {
-		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
 
 		/* PIO commands are handled by polling */
 		ap->hsm_task_state = HSM_ST;

drivers/scsi/libata-scsi.c +2 -2

index ebd7cf4..3628fed 100644
@@ -2146,7 +2146,7 @@ static void atapi_request_sense(struct a
 	ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
 	qc->dma_dir = DMA_FROM_DEVICE;
 
-	memset(&qc->cdb, 0, ap->cdb_len);
+	memset(&qc->cdb, 0, qc->dev->cdb_len);
 	qc->cdb[0] = REQUEST_SENSE;
 	qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;

@@ -2248,7 +2248,7 @@ static unsigned int atapi_xlat(struct at
 		if (ata_check_atapi_dma(qc))
 			using_pio = 1;
 
-	memcpy(&qc->cdb, scsicmd, qc->ap->cdb_len);
+	memcpy(&qc->cdb, scsicmd, dev->cdb_len);
 
 	qc->complete_fn = atapi_qc_complete;

drivers/scsi/sata_sil24.c +2 -2

index 228a7fa..24020c6 100644
@@ -371,7 +371,7 @@ static void sil24_dev_config(struct ata_
 {
 	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
 
-	if (ap->cdb_len == 16)
+	if (dev->cdb_len == 16)
 		writel(PORT_CS_CDB16, port + PORT_CTRL_STAT);
 	else
 		writel(PORT_CS_CDB16, port + PORT_CTRL_CLR);

@@ -543,7 +543,7 @@ static void sil24_qc_prep(struct ata_que
 		prb = &cb->atapi.prb;
 		sge = cb->atapi.sge;
 		memset(cb->atapi.cdb, 0, 32);
-		memcpy(cb->atapi.cdb, qc->cdb, ap->cdb_len);
+		memcpy(cb->atapi.cdb, qc->cdb, qc->dev->cdb_len);
 
 		if (qc->tf.protocol != ATA_PROT_ATAPI_NODATA) {
 			if (qc->tf.flags & ATA_TFLAG_WRITE)

include/linux/libata.h +1 -1

index 0853032..853c988 100644
@@ -348,6 +348,7 @@ struct ata_device {
 
 	unsigned int		multi_count;	/* sectors count for
 						   READ/WRITE MULTIPLE */
+	unsigned int		cdb_len;
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */

@@ -377,7 +378,6 @@ struct ata_port {
 	unsigned int		mwdma_mask;
 	unsigned int		udma_mask;
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
-	unsigned int		cdb_len;
 
 	struct ata_device	device[ATA_MAX_DEVICES];

Signature · 1 line

1.1.5

Tejun Heo

author

2006-02-12 14:33 UTC #4 lore raw

PATCH 3/4 libata: add per-device max_sectors

In reply to Tejun Heo

If a low level driver wants to control max_sectors, it had to adjust ap->host->max_sectors and set ATA_DFLAG_LOCK_SECTORS to tell ata_scsi_slave_config not to override the limit. This is not only cumbersome but also incorrect for hosts which support more than one devices per port.

This patch adds per-device ->max_sectors. If the field is unset (zero), libata core layer will adjust ->max_sectors according to default rules. If the field is set, libata honors the setting.

Signed-off-by Tejun Heo <[email protected]>

---

 drivers/scsi/libata-core.c |    4 +---
 drivers/scsi/libata-scsi.c |   18 +++++++++---------
 drivers/scsi/sata_sil.c    |    4 +---
 include/linux/libata.h     |    4 ++--
 4 files changed, 13 insertions(+), 17 deletions(-)

43ec393ac52f72d2a45f93ee2abf5255bed1d8db

drivers/scsi/libata-core.c +1 -3

index c62798c..4db3db0 100644
@@ -1147,9 +1147,7 @@ void ata_dev_config(struct ata_port *ap,
 		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
 		       ap->id, i);
 		ap->udma_mask &= ATA_UDMA5;
-		ap->host->max_sectors = ATA_MAX_SECTORS;
-		ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
-		ap->device[i].flags |= ATA_DFLAG_LOCK_SECTORS;
+		ap->device[i].max_sectors = ATA_MAX_SECTORS;
 	}
 
 	if (ap->ops->dev_config)

drivers/scsi/libata-scsi.c +9 -9

index 3628fed..86da465 100644
@@ -684,23 +684,23 @@ int ata_scsi_slave_config(struct scsi_de
 	if (sdev->id < ATA_MAX_DEVICES) {
 		struct ata_port *ap;
 		struct ata_device *dev;
+		unsigned int max_sectors;
 
 		ap = (struct ata_port *) &sdev->host->hostdata[0];
 		dev = &ap->device[sdev->id];
 
-		/* TODO: 1024 is an arbitrary number, not the
+		/* TODO: 2048 is an arbitrary number, not the
 		 * hardware maximum.  This should be increased to
 		 * 65534 when Jens Axboe's patch for dynamically
 		 * determining max_sectors is merged.
 		 */
-		if ((dev->flags & ATA_DFLAG_LBA48) &&
-		    ((dev->flags & ATA_DFLAG_LOCK_SECTORS) == 0)) {
-			/*
-			 * do not overwrite sdev->host->max_sectors, since
-			 * other drives on this host may not support LBA48
-			 */
-			blk_queue_max_sectors(sdev->request_queue, 2048);
-		}
+		max_sectors = ATA_MAX_SECTORS;
+		if (dev->flags & ATA_DFLAG_LBA48)
+			max_sectors = 2048;
+		if (dev->max_sectors)
+			max_sectors = dev->max_sectors;
+
+		blk_queue_max_sectors(sdev->request_queue, max_sectors);
 
 		/*
 		 * SATA DMA transfers must be multiples of 4 byte, so

drivers/scsi/sata_sil.c +1 -3

index 61c4ac7..6c482c8 100644
@@ -354,9 +354,7 @@ static void sil_dev_config(struct ata_po
 	     (quirks & SIL_QUIRK_MOD15WRITE))) {
 		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix (mod15write workaround)\n",
 		       ap->id, dev->devno);
-		ap->host->max_sectors = 15;
-		ap->host->hostt->max_sectors = 15;
-		dev->flags |= ATA_DFLAG_LOCK_SECTORS;
+		dev->max_sectors = 15;
 		return;
 	}

include/linux/libata.h +2 -2

index 853c988..afe4645 100644
@@ -122,8 +122,7 @@ enum {
 	/* struct ata_device stuff */
 	ATA_DFLAG_LBA48		= (1 << 0), /* device supports LBA48 */
 	ATA_DFLAG_PIO		= (1 << 1), /* device currently in PIO mode */
-	ATA_DFLAG_LOCK_SECTORS	= (1 << 2), /* don't adjust max_sectors */
-	ATA_DFLAG_LBA		= (1 << 3), /* device supports LBA */
+	ATA_DFLAG_LBA		= (1 << 2), /* device supports LBA */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */

@@ -348,6 +347,7 @@ struct ata_device {
 
 	unsigned int		multi_count;	/* sectors count for
 						   READ/WRITE MULTIPLE */
+	unsigned int		max_sectors;	/* per-device max sectors */
 	unsigned int		cdb_len;
 
 	/* for CHS addressing */

Signature · 1 line

1.1.5

Jeff Garzik

2006-02-12 19:37 UTC #5 lore raw

Re: [PATCH 3/4] libata: add per-device max_sectors

Quoted 58 lines · Tejun Heo

This patch adds per-device ->max_sectors. If the field is unset (zero), libata core layer will adjust ->max_sectors according to default rules. If the field is set, libata honors the setting.

Signed-off-by Tejun Heo <[email protected]>

---

 drivers/scsi/libata-core.c |    4 +---
 drivers/scsi/libata-scsi.c |   18 +++++++++---------
 drivers/scsi/sata_sil.c    |    4 +---
 include/linux/libata.h     |    4 ++--
 4 files changed, 13 insertions(+), 17 deletions(-)

43ec393ac52f72d2a45f93ee2abf5255bed1d8db

drivers/scsi/libata-core.c +1 -3

index c62798c..4db3db0 100644
@@ -1147,9 +1147,7 @@ void ata_dev_config(struct ata_port *ap,
 		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
 		       ap->id, i);
 		ap->udma_mask &= ATA_UDMA5;
-		ap->host->max_sectors = ATA_MAX_SECTORS;
-		ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
-		ap->device[i].flags |= ATA_DFLAG_LOCK_SECTORS;
+		ap->device[i].max_sectors = ATA_MAX_SECTORS;
 	}
 
 	if (ap->ops->dev_config)

drivers/scsi/libata-scsi.c +2 -4

index 3628fed..86da465 100644
@@ -684,23 +684,23 @@ int ata_scsi_slave_config(struct scsi_de
 	if (sdev->id < ATA_MAX_DEVICES) {
 		struct ata_port *ap;
 		struct ata_device *dev;
+		unsigned int max_sectors;
 
 		ap = (struct ata_port *) &sdev->host->hostdata[0];
 		dev = &ap->device[sdev->id];
 
-		/* TODO: 1024 is an arbitrary number, not the
+		/* TODO: 2048 is an arbitrary number, not the
 		 * hardware maximum.  This should be increased to
 		 * 65534 when Jens Axboe's patch for dynamically
 		 * determining max_sectors is merged.
 		 */
-		if ((dev->flags & ATA_DFLAG_LBA48) &&
-		    ((dev->flags & ATA_DFLAG_LOCK_SECTORS) == 0)) {
-			/*

drivers/scsi/libata-scsi.c int ata_scsi_slave_config(struct scsi_de · line 701

Quoted 10 lines

drivers/scsi/libata-scsi.c +5 -5

-			 * do not overwrite sdev->host->max_sectors, since
-			 * other drives on this host may not support LBA48
-			 */
-			blk_queue_max_sectors(sdev->request_queue, 2048);
-		}
+		max_sectors = ATA_MAX_SECTORS;
+		if (dev->flags & ATA_DFLAG_LBA48)
+			max_sectors = 2048;
+		if (dev->max_sectors)
+			max_sectors = dev->max_sectors;

applied 3-4, though note that the Axboe code mentioned in the comment is now in the kernel, so lba48 can be fixed properly.

Jeff

Tejun Heo

author

2006-02-13 00:48 UTC #6 lore raw

Re: [PATCH 3/4] libata: add per-device max_sectors

Quoted 3 lines · Jeff Garzik

applied 3-4, though note that the Axboe code mentioned in the comment is now in the kernel, so lba48 can be fixed properly.

Care to tell me which commit it was? I don't really understand the comment.

Thanks.

Signature · 1 line

tejun

Jeff Garzik

2006-02-13 04:58 UTC #7 lore raw

Re: [PATCH 3/4] libata: add per-device max_sectors

Quoted 7 lines · Tejun Heo

Quoted 3 lines · Jeff Garzik
applied 3-4, though note that the Axboe code mentioned in the comment is now in the kernel, so lba48 can be fixed properly.
Care to tell me which commit it was? I don't really understand the comment.

max_sectors was artificially clipped by the driver due to a system inadequacy: if it truly built commands of 65,536 sectors, then for a full queue, I/O in flight would lock down way too much system memory. Hence, 1024, 2048, 4096 is an arbitrary figure chosen to balance large per-command data size (desireable) and pinned memory (minimize).

The solution was to separate max_sectors -- the maximum we should attach to each command -- with max_hw_sectors, the absolute hardware limit. The system will tune max_sectors up and down, within the max_hw_sectors limit.

Search the kernel changelog for max_hw_sectors.

Jeff

Tejun Heo

author

2006-02-15 07:24 UTC #8 lore raw

PATCH 1/1 libata: increase LBA48 max sectors to 65535

max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.

Signed-off-by Tejun Heo <[email protected]>

---

Jeff, first of all, thanks for the pointer. I have two more questions.

* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?

* ATA_MAX_SECTORS is 200 w/ FIXME. Would bumping it up to 255 be okay? Is this because some lldd's don't handle sectors above 200 very well?

Thanks.

drivers/scsi/libata-scsi.c +1 -6

index 86da465..bceadb3 100644
@@ -689,14 +689,9 @@ int ata_scsi_slave_config(struct scsi_de
 		ap = (struct ata_port *) &sdev->host->hostdata[0];
 		dev = &ap->device[sdev->id];
 
-		/* TODO: 2048 is an arbitrary number, not the
-		 * hardware maximum.  This should be increased to
-		 * 65534 when Jens Axboe's patch for dynamically
-		 * determining max_sectors is merged.
-		 */
 		max_sectors = ATA_MAX_SECTORS;
 		if (dev->flags & ATA_DFLAG_LBA48)
-			max_sectors = 2048;
+			max_sectors = ATA_MAX_SECTORS_LBA48;
 		if (dev->max_sectors)
 			max_sectors = dev->max_sectors;

include/linux/libata.h +1 -0

index afe4645..6253650 100644
@@ -110,6 +110,7 @@ enum {
 	ATA_DEF_QUEUE		= 1,
 	ATA_MAX_QUEUE		= 1,
 	ATA_MAX_SECTORS		= 200,	/* FIXME */
+	ATA_MAX_SECTORS_LBA48	= 65535,
 	ATA_MAX_BUS		= 2,
 	ATA_DEF_BUSY_WAIT	= 10000,
 	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,

Jens Axboe

2006-02-15 13:08 UTC #9 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 5 lines · Tejun Heo

max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>

Quoted 8 lines

---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?

Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.

Signature · 1 line

Jens Axboe

Tejun Heo

author

2006-02-15 15:04 UTC #10 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 17 lines · Jens Axboe

Quoted 5 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?

Quoted 5 lines

Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.

Hello, Jens.

libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.

Hmmm... Can we really trust all the firmwares? I just feel that some drive out there ought have screwed up about nsect == 00h/0000h case.

Thanks.

Signature · 1 line

tejun

Jens Axboe

2006-02-15 15:12 UTC #11 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 24 lines · Tejun Heo

Quoted 17 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?
Quoted 5 lines
Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.

Quoted 5 lines

Hello, Jens.
libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.

Yes I mean the IDE driver. The 200 sector value in libata has always struck me as being extremely odd and a very bad choice. It's somewhere in between reasonable defaults, which isn't very nice. I'd suggest just making it 256 as well, unless Jeff has a reason why it's set to 200.

Quoted 2 lines

Hmmm... Can we really trust all the firmwares? I just feel that some drive out there ought have screwed up about nsect == 00h/0000h case.

Apparently Windows uses it, so I'd say that should pretty much guarantee that it will work for us. And as I mentioned, there has never been a case where the IDE driver value of 256 triggered a drive bug (one time one was suspected, it turned out to be something else though).

Signature · 1 line

Jens Axboe

Tejun Heo

author

2006-02-15 15:32 UTC #12 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 11 lines · Jens Axboe

Quoted 2 lines · Tejun Heo
libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.
Yes I mean the IDE driver. The 200 sector value in libata has always struck me as being extremely odd and a very bad choice. It's somewhere in between reasonable defaults, which isn't very nice. I'd suggest just making it 256 as well, unless Jeff has a reason why it's set to 200.

Quoted 8 lines

Quoted 1 line
Hmmm... Can we really trust all the firmwares? I just feel that some drive out there ought have screwed up about nsect == 00h/0000h case.
Apparently Windows uses it, so I'd say that should pretty much guarantee that it will work for us. And as I mentioned, there has never been a case where the IDE driver value of 256 triggered a drive bug (one time one was suspected, it turned out to be something else though).

Well, then it's Jeff's call. Thanks for the explanation. :-)

Signature · 1 line

tejun

Mark Lord

2006-02-15 19:03 UTC #13 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

In reply to Jens Axboe

There were some PATA host controllers (eg. trm290) that choked on nsects==0, but I've never found a drive that did.

Cheers

Jeff Garzik

2006-02-15 20:21 UTC #14 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

In reply to Jens Axboe

Quoted 30 lines · Jens Axboe

Quoted 28 lines · Tejun Heo
Quoted 18 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?
Quoted 5 lines
Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.
Hello, Jens.

Quoted 7 lines

Quoted 1 line
libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.
Yes I mean the IDE driver. The 200 sector value in libata has always struck me as being extremely odd and a very bad choice. It's somewhere in between reasonable defaults, which isn't very nice. I'd suggest just making it 256 as well, unless Jeff has a reason why it's set to 200.

Its set to 200 for a libata-related reason that I now forget :(

It had nothing to do with controllers or devices, hence the FIXME.

As an aside, I wonder if the PRD limit is unnecessarily low. I don't remember seeing anything that claimed we were limited to 256 s/g entries in the old IDE docs?

Jeff

Mark Lord

2006-02-15 23:07 UTC #15 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 10 lines · Jeff Garzik

Quoted 9 lines · Jens Axboe
Quoted 7 lines · Tejun Heo
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.

Quoted 7 lines

Its set to 200 for a libata-related reason that I now forget :(
It had nothing to do with controllers or devices, hence the FIXME.
As an aside, I wonder if the PRD limit is unnecessarily low. I don't remember seeing anything that claimed we were limited to 256 s/g entries in the old IDE docs?
Jeff

Well, originally I wrote ide-dma.c with a 256 PRD limit, because that corresponds to the absolute worst case of only one sector per PRD. The pre-LBA48 protocol cannot read/write more than that with a single command.

But nowadays..

Cheers

Signature · 3 lines

Mark Lord
Real-Time Remedies Inc.
[email protected]

Jeff Garzik

2006-02-20 11:23 UTC #16 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 9 lines · Mark Lord

Quoted 1 line · Jeff Garzik
As an aside, I wonder if the PRD limit is unnecessarily low. I don't remember seeing anything that claimed we were limited to 256 s/g entries in the old IDE docs?
Well, originally I wrote ide-dma.c with a 256 PRD limit, because that corresponds to the absolute worst case of only one sector per PRD. The pre-LBA48 protocol cannot read/write more than that with a single command.
But nowadays..

Indeed. I am paranoid about controllers assuming that no more than 256 PRD entries will be submitted. But I have never seen any document that says PCI IDE BMDMA hardware can -only- do 256 PRD entries. Never tested it, either...

Jeff

Mark Lord

2006-02-20 13:54 UTC #17 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 2 lines · Jeff Garzik

Indeed. I am paranoid about controllers assuming that no more than 256 PRD entries will be submitted. But I have never seen any document that says PCI IDE BMDMA hardware can -only- do 256 PRD entries. Never tested it, either...

The hardware vendors I have worked with have never had any limit on PRD count -- they'd happily keep reading through an infinite list of PRDs if we could supply one.

A number of them did have issues with 64KB boundary crossings, and it is generally VERY unsafe to have a single PRD "wrap" into a fresh 64KB segment mid-stride.

Cheers

Signature · 3 lines

Mark Lord
Real-Time Remedies Inc.
[email protected]

Jens Axboe

2006-02-20 13:58 UTC #18 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 7 lines · Mark Lord

Quoted 5 lines · Jeff Garzik
Indeed. I am paranoid about controllers assuming that no more than 256 PRD entries will be submitted. But I have never seen any document that says PCI IDE BMDMA hardware can -only- do 256 PRD entries. Never tested it, either...

Quoted 7 lines

The hardware vendors I have worked with have never had any limit on PRD count -- they'd happily keep reading through an infinite list of PRDs if we could supply one.
A number of them did have issues with 64KB boundary crossings, and it is generally VERY unsafe to have a single PRD "wrap" into a fresh 64KB segment mid-stride.

Both IDE and libata take care not to make that happen, so we should be fine there. The block layer also supports this property.

Signature · 1 line

Jens Axboe

Jens Axboe

2006-02-16 07:43 UTC #19 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

In reply to Jeff Garzik

Quoted 36 lines · Jeff Garzik

Quoted 35 lines · Jens Axboe
Quoted 31 lines · Tejun Heo
Quoted 18 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?
Quoted 5 lines
Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.
Hello, Jens.
libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.

Quoted 6 lines

Quoted 4 lines
Yes I mean the IDE driver. The 200 sector value in libata has always struck me as being extremely odd and a very bad choice. It's somewhere in between reasonable defaults, which isn't very nice. I'd suggest just making it 256 as well, unless Jeff has a reason why it's set to 200.
Its set to 200 for a libata-related reason that I now forget :(

Tsk tsk :-)

Quoted 1 line

It had nothing to do with controllers or devices, hence the FIXME.

How about making it 256 at the start of a kernel cycle and see if anything falls apart? If IDE works, then surely SATA should as well.

Quoted 3 lines

As an aside, I wonder if the PRD limit is unnecessarily low. I don't remember seeing anything that claimed we were limited to 256 s/g entries in the old IDE docs?

As Mark mentions it doesn't matter for lba28, for lba48 it'll typically gives us at least 256*4k == 1meg of transfer, which is above/at the soft limit we'll use anyways.

Signature · 1 line

Jens Axboe

Jeff Garzik

2006-02-20 11:25 UTC #20 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 48 lines · Jens Axboe

Quoted 38 lines · Jeff Garzik
Quoted 36 lines · Jens Axboe
Quoted 31 lines · Tejun Heo
Quoted 19 lines · Jens Axboe
Quoted 5 lines · Tejun Heo
max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
---
Jeff, first of all, thanks for the pointer. I have two more questions.
* I make ATA_MAX_SECTORS_LBA48 65535 (the 0000h case is supposed to be broken, right?); however, the comment says 65534. Should it be 65534?
Quoted 5 lines
Since we never noticed any breakage on !sectors on lba28, I think we can safely assume that !sectors will work fine for lba48 as well. So why not just make it full 64k, eg 65536? ->max_hw_sectors is an unsigned int now, so 64k wont overflow it.
Hello, Jens.
libata currently sets max_sectors to 200 (ATA_MAX_SECTORS) on lba28. Are you talking about IDE driver? IDE driver seems to set max_sectors to 256 on probe and make it 2048 while setting up ide disk if lba48.
Quoted 6 lines
Quoted 4 lines
Yes I mean the IDE driver. The 200 sector value in libata has always struck me as being extremely odd and a very bad choice. It's somewhere in between reasonable defaults, which isn't very nice. I'd suggest just making it 256 as well, unless Jeff has a reason why it's set to 200.
Its set to 200 for a libata-related reason that I now forget :(

Quoted 8 lines

Tsk tsk :-)
Quoted 1 line
It had nothing to do with controllers or devices, hence the FIXME.
How about making it 256 at the start of a kernel cycle and see if anything falls apart? If IDE works, then surely SATA should as well.

As I said, it had nothing to do with hardware. The variable in the equation is the driver code itself, so IDE working implies nothing here, alas.

Sigh... I'll search through some old emails.

Jeff

Jens Axboe

2006-02-20 11:44 UTC #21 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

Quoted 8 lines · Jeff Garzik

Quoted 2 lines
How about making it 256 at the start of a kernel cycle and see if anything falls apart? If IDE works, then surely SATA should as well.
As I said, it had nothing to do with hardware. The variable in the equation is the driver code itself, so IDE working implies nothing here, alas.
Sigh... I'll search through some old emails.

Hmmm, I'm having a hard time imagining what such a bug would look like that would cause breakage with 256 sectors and work with 200. Especially since we already do > 256 for lba48. But let me/us know what you dig up from the email archives!

Signature · 1 line

Jens Axboe

Jeff Garzik

Applied

2006-02-20 21:59 UTC #22 lore raw

Re: [PATCH] libata: increase LBA48 max sectors to 65535

In reply to Tejun Heo

Quoted 4 lines · Tejun Heo

max_hw_sectors/max_sectors separation patch made into the tree, increase max_sectors to its hardware limit.
Signed-off-by Tejun Heo <[email protected]>

applied to libata-dev.git#max-sect, and pulled into #ALL.

I want this to get a bit of testing past 2.6.16 release.

Jeff

Tejun Heo

author

2006-02-12 14:33 UTC #23 lore raw

PATCH 4/4 libata: kill sht->max_sectors

In reply to Tejun Heo

The previous dev->max_sectors patch made sht->max_sectors meaningless. Kill all initializations of sht->max_sectors.

Signed-off-by Tejun Heo <[email protected]>

---

 drivers/scsi/ahci.c         |    1 -
 drivers/scsi/ata_piix.c     |    1 -
 drivers/scsi/pdc_adma.c     |    1 -
 drivers/scsi/sata_mv.c      |    1 -
 drivers/scsi/sata_nv.c      |    1 -
 drivers/scsi/sata_promise.c |    1 -
 drivers/scsi/sata_qstor.c   |    1 -
 drivers/scsi/sata_sil.c     |    1 -
 drivers/scsi/sata_sil24.c   |    1 -
 drivers/scsi/sata_sis.c     |    1 -
 drivers/scsi/sata_svw.c     |    1 -
 drivers/scsi/sata_sx4.c     |    1 -
 drivers/scsi/sata_uli.c     |    1 -
 drivers/scsi/sata_via.c     |    1 -
 drivers/scsi/sata_vsc.c     |    1 -
 15 files changed, 0 insertions(+), 15 deletions(-)

05115b54cfe2458ef077e1168533bc14f102fdb3

drivers/scsi/ahci.c +0 -1

index 23caa0c..1c2ab3d 100644
@@ -211,7 +211,6 @@ static struct scsi_host_template ahci_sh
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= AHCI_MAX_SG,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= AHCI_USE_CLUSTERING,

drivers/scsi/ata_piix.c +0 -1

index 4933ba2..289be58 100644
@@ -185,7 +185,6 @@ static struct scsi_host_template piix_sh
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/pdc_adma.c +0 -1

index d0ad3eb..5f33cc9 100644
@@ -148,7 +148,6 @@ static struct scsi_host_template adma_at
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ENABLE_CLUSTERING,

drivers/scsi/sata_mv.c +0 -1

index 732cb64..c158de2 100644
@@ -383,7 +383,6 @@ static struct scsi_host_template mv_sht 
 	.can_queue		= MV_USE_Q_DEPTH,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= MV_MAX_SG_CT / 2,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_nv.c +0 -1

index cdfeb9a..caffadc 100644
@@ -234,7 +234,6 @@ static struct scsi_host_template nv_sht 
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_promise.c +0 -1

index c9dfd93..ba2b7a0 100644
@@ -116,7 +116,6 @@ static struct scsi_host_template pdc_ata
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_qstor.c +0 -1

index 955131b..2864826 100644
@@ -137,7 +137,6 @@ static struct scsi_host_template qs_ata_
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= QS_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	//FIXME .use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_sil.c +0 -1

index 6c482c8..1534688 100644
@@ -140,7 +140,6 @@ static struct scsi_host_template sil_sht
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_sil24.c +0 -1

index 24020c6..a0e35a2 100644
@@ -285,7 +285,6 @@ static struct scsi_host_template sil24_s
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_sis.c +0 -1

index 2f18157..7fd45f8 100644
@@ -92,7 +92,6 @@ static struct scsi_host_template sis_sht
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= ATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_svw.c +0 -1

index f369c30..4aaccd5 100644
@@ -293,7 +293,6 @@ static struct scsi_host_template k2_sata
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_sx4.c +0 -1

index e158f7a..9f8a768 100644
@@ -187,7 +187,6 @@ static struct scsi_host_template pdc_sat
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_uli.c +0 -1

index c500f24..37a487b 100644
@@ -80,7 +80,6 @@ static struct scsi_host_template uli_sht
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_via.c +0 -1

index 2e20887..ff65a0b 100644
@@ -99,7 +99,6 @@ static struct scsi_host_template svia_sh
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

drivers/scsi/sata_vsc.c +0 -1

index cf1f8a6..e124db8 100644
@@ -228,7 +228,6 @@ static struct scsi_host_template vsc_sat
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
 	.sg_tablesize		= LIBATA_MAX_PRD,
-	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
 	.use_clustering		= ATA_SHT_USE_CLUSTERING,

Signature · 1 line

1.1.5