Skip to contentSkip to conversation

libata: implement per-dev xfer masks, take#2

PATCH New Tejun Heo 2 participants 8 messages Mar 24, 2006 – Mar 25, 2006

Subsystems (2)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: implement per-dev xfer masks, take#2

Hello, all.

This is the second take of add-per-dev-pio-mwdma-udma_mask patchset. Changes from the last take[1] are...

* %02 implement-ata_dev_init patch is removed and %03 is updated accordingly.

The patchset is against the current #upstream[2].

I tried to think of a simple way to use single xfer_mask for all masks, but couldn't. The problem is that the masks are used in several different ways affecting specific parts of the mask. When composing transfer xfer_mask, it's best if they can be simply bit-wise or'ed, but we can't use the same mask values when trying to limit trasnfer mode because bits for other transfer modes will be turned off. We're gonna have to use different masks for different purposes or use mask manipulating wrappers.

So, for the time being, LLDD's are left with three separate masks. If anyone can think of a pretty way to convert them, please, go ahead. We also need to sort out transfer mask constants, the naming is somewhat inconsistent.

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/8705
[2] 2cc432eed0491df66e14b578139bba2c75fb3f9a

author
#1 lore raw

PATCH 1/3 libata: implement ata_unpack_xfermask()

Implement ata_unpack_xfermask().

---

 drivers/scsi/libata-core.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

6ff994d5a30bdaef8a055301f569b781307801a7

drivers/scsi/libata-core.c +23 -0
index f3c115b..1514cb5 100644
@@ -252,6 +252,29 @@ static unsigned int ata_pack_xfermask(un
 		((udma_mask << ATA_SHIFT_UDMA) & ATA_MASK_UDMA);
 }
 
+/**
+ *	ata_unpack_xfermask - Unpack xfer_mask into pio, mwdma and udma masks
+ *	@xfer_mask: xfer_mask to unpack
+ *	@pio_mask: resulting pio_mask
+ *	@mwdma_mask: resulting mwdma_mask
+ *	@udma_mask: resulting udma_mask
+ *
+ *	Unpack @xfer_mask into @pio_mask, @mwdma_mask and @udma_mask.
+ *	Any NULL distination masks will be ignored.
+ */
+static void ata_unpack_xfermask(unsigned int xfer_mask,
+				unsigned int *pio_mask,
+				unsigned int *mwdma_mask,
+				unsigned int *udma_mask)
+{
+	if (pio_mask)
+		*pio_mask = (xfer_mask & ATA_MASK_PIO) >> ATA_SHIFT_PIO;
+	if (mwdma_mask)
+		*mwdma_mask = (xfer_mask & ATA_MASK_MWDMA) >> ATA_SHIFT_MWDMA;
+	if (udma_mask)
+		*udma_mask = (xfer_mask & ATA_MASK_UDMA) >> ATA_SHIFT_UDMA;
+}
+
 static const struct ata_xfer_ent {
 	unsigned int shift, bits;
 	u8 base;
Signature · 1 line
1.2.4
author
#2 lore raw

PATCH 2/3 libata: add per-dev pio/mwdma/udma_mask

In reply to Tejun Heo

Add per-dev pio/mwdma/udma_mask. All transfer mode limits used to be applied to ap->*_mask which unnecessarily restricted other devices sharing the port. This change will also benefit later EH speed down and hotplug.

---

 drivers/scsi/libata-core.c |   43 +++++++++++++++++++++++++------------------
 include/linux/libata.h     |    5 +++++
 2 files changed, 30 insertions(+), 18 deletions(-)

31c02ad0eeb7c73243a109a75e41b09511c1576f

drivers/scsi/libata-core.c +25 -18
index 1514cb5..a87748b 100644
@@ -65,8 +65,7 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev);
 static void ata_set_mode(struct ata_port *ap);
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
-static unsigned int ata_dev_xfermask(struct ata_port *ap,
-				     struct ata_device *dev);
+static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -1801,16 +1800,19 @@ static void ata_set_mode(struct ata_port
 	/* step 1: calculate xfer_mask */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
-		unsigned int xfer_mask;
+		unsigned int pio_mask, dma_mask;
 
 		if (!ata_dev_present(dev))
 			continue;
 
-		xfer_mask = ata_dev_xfermask(ap, dev);
+		ata_dev_xfermask(ap, dev);
 
-		dev->pio_mode = ata_xfer_mask2mode(xfer_mask & ATA_MASK_PIO);
-		dev->dma_mode = ata_xfer_mask2mode(xfer_mask & (ATA_MASK_MWDMA |
-								ATA_MASK_UDMA));
+		/* TODO: let LLDD filter dev->*_mask here */
+
+		pio_mask = ata_pack_xfermask(dev->pio_mask, 0, 0);
+		dma_mask = ata_pack_xfermask(0, dev->mwdma_mask, dev->udma_mask);
+		dev->pio_mode = ata_xfer_mask2mode(pio_mask);
+		dev->dma_mode = ata_xfer_mask2mode(dma_mask);
 	}
 
 	/* step 2: always set host PIO timings */
@@ -2653,18 +2655,15 @@ static int ata_dma_blacklisted(const str
  *	@ap: Port on which the device to compute xfermask for resides
  *	@dev: Device to compute xfermask for
  *
- *	Compute supported xfermask of @dev.  This function is
- *	responsible for applying all known limits including host
- *	controller limits, device blacklist, etc...
+ *	Compute supported xfermask of @dev and store it in
+ *	dev->*_mask.  This function is responsible for applying all
+ *	known limits including host controller limits, device
+ *	blacklist, etc...
  *
  *	LOCKING:
  *	None.
- *
- *	RETURNS:
- *	Computed xfermask.
  */
-static unsigned int ata_dev_xfermask(struct ata_port *ap,
-				     struct ata_device *dev)
+static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev)
 {
 	unsigned long xfer_mask;
 	int i;
@@ -2677,6 +2676,8 @@ static unsigned int ata_dev_xfermask(str
 		struct ata_device *d = &ap->device[i];
 		if (!ata_dev_present(d))
 			continue;
+		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
+					       d->udma_mask);
 		xfer_mask &= ata_id_xfermask(d->id);
 		if (ata_dma_blacklisted(d))
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
@@ -2686,7 +2687,8 @@ static unsigned int ata_dev_xfermask(str
 		printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, "
 		       "disabling DMA\n", ap->id, dev->devno);
 
-	return xfer_mask;
+	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
+			    &dev->udma_mask);
 }
 
 /**
@@ -4436,8 +4438,13 @@ static void ata_host_init(struct ata_por
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ap->device[i].devno = i;
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
+		dev->devno = i;
+		dev->pio_mask = UINT_MAX;
+		dev->mwdma_mask = UINT_MAX;
+		dev->udma_mask = UINT_MAX;
+	}
 
 #ifdef ATA_IRQ_TRAP
 	ap->stats.unhandled_irq = 1;
include/linux/libata.h +5 -0
index 7a54244..fbe8ba2 100644
@@ -358,6 +358,11 @@ struct ata_device {
 	unsigned int		max_sectors;	/* per-device max sectors */
 	unsigned int		cdb_len;
 
+	/* per-dev xfer mask */
+	unsigned int		pio_mask;
+	unsigned int		mwdma_mask;
+	unsigned int		udma_mask;
+
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
 	u16			heads;		/* Number of heads */
Signature · 1 line
1.2.4
#3 lore raw

Re: [PATCH 2/3] libata: add per-dev pio/mwdma/udma_mask

Quoted 16 lines · Tejun Heo
drivers/scsi/libata-core.c +3 -2
-static unsigned int ata_dev_xfermask(struct ata_port *ap,
-				     struct ata_device *dev)
+static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev)
 {
 	unsigned long xfer_mask;
 	int i;
@@ -2677,6 +2676,8 @@ static unsigned int ata_dev_xfermask(str
 		struct ata_device *d = &ap->device[i];
 		if (!ata_dev_present(d))
 			continue;
+		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
+					       d->udma_mask);
 		xfer_mask &= ata_id_xfermask(d->id);
 		if (ata_dma_blacklisted(d))
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);

drivers/scsi/libata-core.c static unsigned int ata_dev_xfermask(str · line 2691

Quoted 6 lines
drivers/scsi/libata-core.c +2 -1
@@ -2686,7 +2687,8 @@ static unsigned int ata_dev_xfermask(str
 		printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, "
 		       "disabling DMA\n", ap->id, dev->devno);
 
-	return xfer_mask;
+	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
+			    &dev->udma_mask);

applied 1-3. You should add a TODO note somewhere, that notes that the current implementation intentionally limits all devices on a single port to the fastest mode on the slowest device. This is necessary for SOME controllers but not all, and so we shouldn't do it unconditionally.

Jeff

author
#4 lore raw

PATCH 1/1 libata: add FIXME above ata_dev_xfermask()

Add FIXME above ata_dev_xfermask noting that the current implementation limits all transfer modes to the fastest of the slowest device on a port which isn't necessary.

drivers/scsi/libata-core.c +4 -0
index 0aff888..e4bd673 100644
@@ -2634,6 +2634,10 @@ static int ata_dma_blacklisted(const str
  *	known limits including host controller limits, device
  *	blacklist, etc...
  *
+ *	FIXME: The current implementation limits all transfer modes to
+ *	the fastest of the lowested device on the port.  This is not
+ *	required on most controllers. 
+ *
  *	LOCKING:
  *	None.
  */
Applied
#5 lore raw

Re: [PATCH] libata: add FIXME above ata_dev_xfermask()

Quoted 11 lines · Tejun Heo

Add FIXME above ata_dev_xfermask noting that the current implementation limits all transfer modes to the fastest of the slowest device on a port which isn't necessary.

drivers/scsi/libata-core.c +0 -0
index 0aff888..e4bd673 100644

drivers/scsi/libata-core.c static int ata_dma_blacklisted(const str · line 2639

Quoted 6 lines
drivers/scsi/libata-core.c +3 -0
@@ -2634,6 +2634,10 @@ static int ata_dma_blacklisted(const str
  *	known limits including host controller limits, device
  *	blacklist, etc...
  *
+ *	FIXME: The current implementation limits all transfer modes to
+ *	the fastest of the lowested device on the port.  This is not
+ *	required on most controllers. 

applied, though git complained about...

[jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt 1 patch(es) to process.

Applying 'libata: add FIXME above ata_dev_xfermask()'

Adds trailing whitespace.
.dotest/patch:11: * required on most controllers.
warning: 1 line adds trailing whitespaces. Wrote tree 0fc6b51c6ddfb5d3f759bb88a34d8003386a50d7
Committed: 600511e86babe3727264a0883a3a264f6fb6caf5

author
#6 lore raw

PATCH 1/1 libata: kill trailing whitespace

Kill trailing whitespace.

---

Sorry about that.

drivers/scsi/libata-core.c +1 -1
index 1cb9813..8631056 100644
@@ -2635,7 +2635,7 @@ static int ata_dma_blacklisted(const str
  *
  *	FIXME: The current implementation limits all transfer modes to
  *	the fastest of the lowested device on the port.  This is not
- *	required on most controllers. 
+ *	required on most controllers.
  *
  *	LOCKING:
  *	None.
author
#7 lore raw

PATCH 3/3 libata: make per-dev transfer mode limits per-dev

In reply to Tejun Heo

Now that each ata_device has xfer masks, per-dev limits can be made per-dev instead of per-port. Make per-dev limits per-dev.

---

 drivers/scsi/libata-core.c |    2 +-
 drivers/scsi/sata_sil.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

d1e4136f124dfae288a5dd382e9d54866a355a44

drivers/scsi/libata-core.c +1 -1
index a87748b..1063928 100644
@@ -1327,7 +1327,7 @@ static int ata_dev_configure(struct ata_
 		if (print_info)
 			printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
 			       ap->id, dev->devno);
-		ap->udma_mask &= ATA_UDMA5;
+		dev->udma_mask &= ATA_UDMA5;
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
 
drivers/scsi/sata_sil.c +1 -1
index 3e75d67..18c296c 100644
@@ -371,7 +371,7 @@ static void sil_dev_config(struct ata_po
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
 		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",
 		       ap->id, dev->devno, model_num);
-		ap->udma_mask &= ATA_UDMA5;
+		dev->udma_mask &= ATA_UDMA5;
 		return;
 	}
 }
Signature · 1 line
1.2.4