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

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

[Tejun Heo]

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

[PATCH 1/3] libata: implement ata_unpack_xfermask()

[Tejun Heo]

Implement ata_unpack_xfermask().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

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

6ff994d5a30bdaef8a055301f569b781307801a7
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f3c115b..1514cb5 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -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;
-- 
1.2.4

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

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

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

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

31c02ad0eeb7c73243a109a75e41b09511c1576f
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1514cb5..a87748b 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -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;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7a54244..fbe8ba2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */
-- 
1.2.4

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

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

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

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

d1e4136f124dfae288a5dd382e9d54866a355a44
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index a87748b..1063928 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -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;
 	}
 
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 3e75d67..18c296c 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -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;
 	}
 }
-- 
1.2.4

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

[Jeff Garzik]

Tejun Heo wrote:
> -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);

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

[PATCH] libata: add FIXME above ata_dev_xfermask()

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

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 0aff888..e4bd673 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -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.
  */

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

[Jeff Garzik]

Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 0aff888..e4bd673 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -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

[PATCH] libata: kill trailing whitespace

[Tejun Heo]

Kill trailing whitespace.

---

Sorry about that.

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1cb9813..8631056 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -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.