[PATCHSET] libata: improve transfer mode handling

[PATCHSET] libata: improve transfer mode handling

[Tejun Heo]

Hello, Jeff, Alan and Albert.

This is the second take of improve-transfer-mode-handling patchset.
The last take[1] made low level driver API uglier and Alan rejected
the API changes.  Changes from the last take are...

* No low level driver API change.  All changes are confined tolibata
  core layer proper.

* Fewer/simpler xfer_mask helpers.

* Better splitted patches.

This patchset makes transfer mode determination done in
ata_dev_xfermask() before any of actual configuration happens.  This
should help integrating Alan's changes easier.

This patchset is against the current upstream[2] + port_task
patchset[3].  However, all the patches except the last one applies
against upstream with offsets and the reject of the last patch is
trivial.  I'll post two versions of the last patch - one against
upstream + port_task, the other against upstream.

Thanks.

--
tejun

[1] http://marc.theaimsgroup.com/?l=linux-ide&m=114009896413075&w=2
[2] fbfda6e71bbdd3b4d41a56c3f20f31762c455a5e
[3] http://marc.theaimsgroup.com/?l=linux-ide&m=114154013813744&w=2
(dang, gmane still down.)

[PATCH 1/6] libata: improve xfer mask constants and update ata_mode_string()

[Tejun Heo]

Add ATA_BITS_*, ATA_MASK_* macros and reorder xfer_mask fields such
that higher transfer mode is placed at higher order bit.  As thie
reordering breaks ata_mode_string(), this patch also rewrites
ata_mode_string().

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

---

 drivers/scsi/libata-core.c |   44 +++++++++++++++++---------------------------
 include/linux/libata.h     |   16 ++++++++++++----
 2 files changed, 29 insertions(+), 31 deletions(-)

ae787be679d5411a1877134ba7d2cfbc7e1cdb69
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 6d8aa86..18418c8 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -232,6 +232,14 @@ int ata_rwcmd_protocol(struct ata_queued
 }
 
 static const char * const xfer_mode_str[] = {
+	"PIO0",
+	"PIO1",
+	"PIO2",
+	"PIO3",
+	"PIO4",
+	"MWDMA0",
+	"MWDMA1",
+	"MWDMA2",
 	"UDMA/16",
 	"UDMA/25",
 	"UDMA/33",
@@ -240,49 +248,31 @@ static const char * const xfer_mode_str[
 	"UDMA/100",
 	"UDMA/133",
 	"UDMA7",
-	"MWDMA0",
-	"MWDMA1",
-	"MWDMA2",
-	"PIO0",
-	"PIO1",
-	"PIO2",
-	"PIO3",
-	"PIO4",
 };
 
 /**
- *	ata_udma_string - convert UDMA bit offset to string
- *	@mask: mask of bits supported; only highest bit counts.
+ *	ata_mode_string - convert xfer_mask to string
+ *	@xfer_mask: mask of bits supported; only highest bit counts.
  *
  *	Determine string which represents the highest speed
- *	(highest bit in @udma_mask).
+ *	(highest bit in @modemask).
  *
  *	LOCKING:
  *	None.
  *
  *	RETURNS:
  *	Constant C string representing highest speed listed in
- *	@udma_mask, or the constant C string "<n/a>".
+ *	@mode_mask, or the constant C string "<n/a>".
  */
 
-static const char *ata_mode_string(unsigned int mask)
+static const char *ata_mode_string(unsigned int xfer_mask)
 {
-	int i;
-
-	for (i = 7; i >= 0; i--)
-		if (mask & (1 << i))
-			goto out;
-	for (i = ATA_SHIFT_MWDMA + 2; i >= ATA_SHIFT_MWDMA; i--)
-		if (mask & (1 << i))
-			goto out;
-	for (i = ATA_SHIFT_PIO + 4; i >= ATA_SHIFT_PIO; i--)
-		if (mask & (1 << i))
-			goto out;
+	int highbit;
 
+	highbit = fls(xfer_mask) - 1;
+	if (highbit >= 0 && highbit < ARRAY_SIZE(xfer_mode_str))
+		return xfer_mode_str[highbit];
 	return "<n/a>";
-
-out:
-	return xfer_mode_str[i];
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1567492..239408e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -188,11 +188,19 @@ enum {
 	PORT_DISABLED		= 2,
 
 	/* encoding various smaller bitmaps into a single
-	 * unsigned long bitmap
+	 * unsigned int bitmap
 	 */
-	ATA_SHIFT_UDMA		= 0,
-	ATA_SHIFT_MWDMA		= 8,
-	ATA_SHIFT_PIO		= 11,
+	ATA_BITS_PIO		= 5,
+	ATA_BITS_MWDMA		= 3,
+	ATA_BITS_UDMA		= 8,
+
+	ATA_SHIFT_PIO		= 0,
+	ATA_SHIFT_MWDMA		= ATA_SHIFT_PIO + ATA_BITS_PIO,
+	ATA_SHIFT_UDMA		= ATA_SHIFT_MWDMA + ATA_BITS_MWDMA,
+
+	ATA_MASK_PIO		= ((1 << ATA_BITS_PIO) - 1) << ATA_SHIFT_PIO,
+	ATA_MASK_MWDMA		= ((1 << ATA_BITS_MWDMA) - 1) << ATA_SHIFT_MWDMA,
+	ATA_MASK_UDMA		= ((1 << ATA_BITS_UDMA) - 1) << ATA_SHIFT_UDMA,
 
 	/* size of buffer to pad xfers ending on unaligned boundaries */
 	ATA_DMA_PAD_SZ		= 4,
-- 
1.2.4

Re: [PATCH 1/6] libata: improve xfer mask constants and update ata_mode_string()

[Jeff Garzik]

Tejun Heo wrote:
> Add ATA_BITS_*, ATA_MASK_* macros and reorder xfer_mask fields such
> that higher transfer mode is placed at higher order bit.  As thie
> reordering breaks ata_mode_string(), this patch also rewrites
> ata_mode_string().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-6, please send follow-up patch per comments on patch #2

[PATCH 2/6] libata: add xfer_mask handling functions

[Tejun Heo]

Add ata_pack_xfermask(), ata_xfer_mask2mode(), ata_xfer_mode2mask(),
ata_xfer_mode2shift() and ata_id_xfermask().  These functions will be
used by following patches to simplify xfer_mask handling.

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

---

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

3247748741bb32ca18e3fa3c0dd14690dbd184a6
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 18418c8..9946618 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -231,6 +231,108 @@ int ata_rwcmd_protocol(struct ata_queued
 	return -1;
 }
 
+/**
+ *	ata_pack_xfermask - Pack pio, mwdma and udma masks into xfer_mask
+ *	@pio_mask: pio_mask
+ *	@mwdma_mask: mwdma_mask
+ *	@udma_mask: udma_mask
+ *
+ *	Pack @pio_mask, @mwdma_mask and @udma_mask into a single
+ *	unsigned int xfer_mask.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Packed xfer_mask.
+ */
+static unsigned int ata_pack_xfermask(unsigned int pio_mask,
+				      unsigned int mwdma_mask,
+				      unsigned int udma_mask)
+{
+	return ((pio_mask << ATA_SHIFT_PIO) & ATA_MASK_PIO) |
+		((mwdma_mask << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA) |
+		((udma_mask << ATA_SHIFT_UDMA) & ATA_MASK_UDMA);
+}
+
+static const struct ata_xfer_ent {
+	unsigned int shift, bits;
+	u8 base;
+} ata_xfer_tbl[] = {
+	{ ATA_SHIFT_PIO, ATA_BITS_PIO, XFER_PIO_0 },
+	{ ATA_SHIFT_MWDMA, ATA_BITS_MWDMA, XFER_MW_DMA_0 },
+	{ ATA_SHIFT_UDMA, ATA_BITS_UDMA, XFER_UDMA_0 },
+	{ -1, },
+};
+
+/**
+ *	ata_xfer_mask2mode - Find matching XFER_* for the given xfer_mask
+ *	@xfer_mask: xfer_mask of interest
+ *
+ *	Return matching XFER_* value for @xfer_mask.  Only the highest
+ *	bit of @xfer_mask is considered.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Matching XFER_* value, 0 if no match found.
+ */
+static u8 ata_xfer_mask2mode(unsigned int xfer_mask)
+{
+	int highbit = fls(xfer_mask) - 1;
+	const struct ata_xfer_ent *ent;
+
+	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
+		if (highbit >= ent->shift && highbit < ent->shift + ent->bits)
+			return ent->base + highbit - ent->shift;
+	return 0;
+}
+
+/**
+ *	ata_xfer_mode2mask - Find matching xfer_mask for XFER_*
+ *	@xfer_mode: XFER_* of interest
+ *
+ *	Return matching xfer_mask for @xfer_mode.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Matching xfer_mask, 0 if no match found.
+ */
+static unsigned int ata_xfer_mode2mask(u8 xfer_mode)
+{
+	const struct ata_xfer_ent *ent;
+
+	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
+		if (xfer_mode >= ent->base && xfer_mode < ent->base + ent->bits)
+			return 1 << (ent->shift + xfer_mode - ent->base);
+	return 0;
+}
+
+/**
+ *	ata_xfer_mode2shift - Find matching xfer_shift for XFER_*
+ *	@xfer_mode: XFER_* of interest
+ *
+ *	Return matching xfer_shift for @xfer_mode.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Matching xfer_shift, -1 if no match found.
+ */
+static int ata_xfer_mode2shift(unsigned int xfer_mode)
+{
+	const struct ata_xfer_ent *ent;
+
+	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
+		if (xfer_mode >= ent->base && xfer_mode < ent->base + ent->bits)
+			return ent->shift;
+	return -1;
+}
+
 static const char * const xfer_mode_str[] = {
 	"PIO0",
 	"PIO1",
@@ -682,6 +784,51 @@ static inline void ata_dump_id(const u16
 		id[93]);
 }
 
+/**
+ *	ata_id_xfermask - Compute xfermask from the given IDENTIFY data
+ *	@id: IDENTIFY data to compute xfer mask from
+ *
+ *	Compute the xfermask for this device. This is not as trivial
+ *	as it seems if we must consider early devices correctly.
+ *
+ *	FIXME: pre IDE drive timing (do we care ?).
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Computed xfermask
+ */
+static unsigned int ata_id_xfermask(const u16 *id)
+{
+	unsigned int pio_mask, mwdma_mask, udma_mask;
+
+	/* Usual case. Word 53 indicates word 64 is valid */
+	if (id[ATA_ID_FIELD_VALID] & (1 << 1)) {
+		pio_mask = id[ATA_ID_PIO_MODES] & 0x03;
+		pio_mask <<= 3;
+		pio_mask |= 0x7;
+	} else {
+		/* If word 64 isn't valid then Word 51 high byte holds
+		 * the PIO timing number for the maximum. Turn it into
+		 * a mask.
+		 */
+		pio_mask = (2 << (id[ATA_ID_OLD_PIO_MODES] & 0xFF)) - 1 ;
+
+		/* But wait.. there's more. Design your standards by
+		 * committee and you too can get a free iordy field to
+		 * process. However its the speeds not the modes that
+		 * are supported... Note drivers using the timing API
+		 * will get this right anyway
+		 */
+	}
+
+	mwdma_mask = id[ATA_ID_MWDMA_MODES] & 0x07;
+	udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;
+
+	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
+}
+
 /*
  *	Compute the PIO modes available for this device. This is not as
  *	trivial as it seems if we must consider early devices correctly.
-- 
1.2.4

Re: [PATCH 2/6] libata: add xfer_mask handling functions

[Albert Lee]

Tejun Heo wrote:
> Add ata_pack_xfermask(), ata_xfer_mask2mode(), ata_xfer_mode2mask(),
> ata_xfer_mode2shift() and ata_id_xfermask().  These functions will be
> used by following patches to simplify xfer_mask handling.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
>  (snip)
> +/**
> + *	ata_id_xfermask - Compute xfermask from the given IDENTIFY data
> + *	@id: IDENTIFY data to compute xfer mask from
> + *
> + *	Compute the xfermask for this device. This is not as trivial
> + *	as it seems if we must consider early devices correctly.
> + *
> + *	FIXME: pre IDE drive timing (do we care ?).
> + *
> + *	LOCKING:
> + *	None.
> + *
> + *	RETURNS:
> + *	Computed xfermask
> + */
> +static unsigned int ata_id_xfermask(const u16 *id)
> +{
> +	unsigned int pio_mask, mwdma_mask, udma_mask;
> +
> +	/* Usual case. Word 53 indicates word 64 is valid */
> +	if (id[ATA_ID_FIELD_VALID] & (1 << 1)) {
> +		pio_mask = id[ATA_ID_PIO_MODES] & 0x03;
> +		pio_mask <<= 3;
> +		pio_mask |= 0x7;
> +	} else {
> +		/* If word 64 isn't valid then Word 51 high byte holds
> +		 * the PIO timing number for the maximum. Turn it into
> +		 * a mask.
> +		 */
> +		pio_mask = (2 << (id[ATA_ID_OLD_PIO_MODES] & 0xFF)) - 1 ;
> +
> +		/* But wait.. there's more. Design your standards by
> +		 * committee and you too can get a free iordy field to
> +		 * process. However its the speeds not the modes that
> +		 * are supported... Note drivers using the timing API
> +		 * will get this right anyway
> +		 */
> +	}
> +
> +	mwdma_mask = id[ATA_ID_MWDMA_MODES] & 0x07;
> +	udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;
> +
> +	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
> +}
> +
>  /*

We have ap->pio_mask, ap->mwdma_mask and ap->udma_mask.
Just thinking what if we have these masks in ata_device? 
Maybe we can save some bitwise operations and make the code more intuitive to read?

Ex. ata_id_xfermask() can store the calculated masks to dev->pio_mask, dev->mwdma_mask, etc.
Ex. ata_mode_string() can take mode (XFER_UDMA_7..) as parameter and translate the given mode to string.
    
Ex. to print out the max mode support by the device, 
    1. ata_id_xfermask() calculates and saves the masks to dev->pio_mask, etc.
    2. Another function, say, ata_dev_max_mode() takes dev as parameter,
       packs the mode by ata_pack_xfermask() internally,
       find the max mode supported by the drive,
       then use ata_xfer_mask2mode() to return the mode.
    3. ata_mode_string() translates the mode, say XFER_UDMA_7, to string literal.

Albert

Re: [PATCH 2/6] libata: add xfer_mask handling functions

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
> 
>>Add ata_pack_xfermask(), ata_xfer_mask2mode(), ata_xfer_mode2mask(),
>>ata_xfer_mode2shift() and ata_id_xfermask().  These functions will be
>>used by following patches to simplify xfer_mask handling.
>>
>>Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> (snip)
>>+/**
>>+ *	ata_id_xfermask - Compute xfermask from the given IDENTIFY data
>>+ *	@id: IDENTIFY data to compute xfer mask from
>>+ *
>>+ *	Compute the xfermask for this device. This is not as trivial
>>+ *	as it seems if we must consider early devices correctly.
>>+ *
>>+ *	FIXME: pre IDE drive timing (do we care ?).
>>+ *
>>+ *	LOCKING:
>>+ *	None.
>>+ *
>>+ *	RETURNS:
>>+ *	Computed xfermask
>>+ */
>>+static unsigned int ata_id_xfermask(const u16 *id)
>>+{
>>+	unsigned int pio_mask, mwdma_mask, udma_mask;
>>+
>>+	/* Usual case. Word 53 indicates word 64 is valid */
>>+	if (id[ATA_ID_FIELD_VALID] & (1 << 1)) {
>>+		pio_mask = id[ATA_ID_PIO_MODES] & 0x03;
>>+		pio_mask <<= 3;
>>+		pio_mask |= 0x7;
>>+	} else {
>>+		/* If word 64 isn't valid then Word 51 high byte holds
>>+		 * the PIO timing number for the maximum. Turn it into
>>+		 * a mask.
>>+		 */
>>+		pio_mask = (2 << (id[ATA_ID_OLD_PIO_MODES] & 0xFF)) - 1 ;
>>+
>>+		/* But wait.. there's more. Design your standards by
>>+		 * committee and you too can get a free iordy field to
>>+		 * process. However its the speeds not the modes that
>>+		 * are supported... Note drivers using the timing API
>>+		 * will get this right anyway
>>+		 */
>>+	}
>>+
>>+	mwdma_mask = id[ATA_ID_MWDMA_MODES] & 0x07;
>>+	udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;
>>+
>>+	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
>>+}
>>+
>> /*
> 
> 
> We have ap->pio_mask, ap->mwdma_mask and ap->udma_mask.
> Just thinking what if we have these masks in ata_device? 

We need those masks in both ap and dev. ap capability should be stored 
in ap->*_mask and dev capabilities in dev->*_mask. e.g. speed limits due 
to controller quirks should be done by limiting ap->*_mask in 
->dev_config() while device speeding down due to transmission errors 
should be done by limiting dev->*_mask.

> Maybe we can save some bitwise operations and make the code more intuitive to read?
> 
> Ex. ata_id_xfermask() can store the calculated masks to dev->pio_mask, dev->mwdma_mask, etc.

This will be done when adding dev->*_mask.

> Ex. ata_mode_string() can take mode (XFER_UDMA_7..) as parameter and translate the given mode to string.

Hmmm... I don't know. My plan is to unify transfer mode handling to 
unsigned int xfer_mask in libata core layer proper. It's easier to pass 
around, mask, manipulate, etc... That's why I made ata_mode_string() 
take xfer_mask instead of mode constants. But I'm okay either way. 
Converting back and forth between xfermask and mode constant isn't 
difficult.

> Ex. to print out the max mode support by the device, 
>     1. ata_id_xfermask() calculates and saves the masks to dev->pio_mask, etc.
>     2. Another function, say, ata_dev_max_mode() takes dev as parameter,
>        packs the mode by ata_pack_xfermask() internally,
>        find the max mode supported by the drive,
>        then use ata_xfer_mask2mode() to return the mode.
>     3. ata_mode_string() translates the mode, say XFER_UDMA_7, to string literal.

Hmm.. 1, 2 and 3 can just be done like the following.

printk("max speed=%s\n", ata_mode_string(ata_id_xfermask(id));

I think it's simpler this way. No?

-- 
tejun

Re: [PATCH 2/6] libata: add xfer_mask handling functions

[Jeff Garzik]

Tejun Heo wrote:
> +static unsigned int ata_id_xfermask(const u16 *id)
> +{
> +	unsigned int pio_mask, mwdma_mask, udma_mask;
> +
> +	/* Usual case. Word 53 indicates word 64 is valid */
> +	if (id[ATA_ID_FIELD_VALID] & (1 << 1)) {
> +		pio_mask = id[ATA_ID_PIO_MODES] & 0x03;
> +		pio_mask <<= 3;
> +		pio_mask |= 0x7;
> +	} else {
> +		/* If word 64 isn't valid then Word 51 high byte holds
> +		 * the PIO timing number for the maximum. Turn it into
> +		 * a mask.
> +		 */
> +		pio_mask = (2 << (id[ATA_ID_OLD_PIO_MODES] & 0xFF)) - 1 ;
> +
> +		/* But wait.. there's more. Design your standards by
> +		 * committee and you too can get a free iordy field to
> +		 * process. However its the speeds not the modes that
> +		 * are supported... Note drivers using the timing API
> +		 * will get this right anyway
> +		 */
> +	}
> +
> +	mwdma_mask = id[ATA_ID_MWDMA_MODES] & 0x07;
> +	udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;

Check Word 53 to see if Word 88 is valid.

Otherwise OK.

	Jeff

[PATCH] libata: check Word 88 validity in ata_id_xfer_mask()

[Tejun Heo]

Check bit 2 of Word 53 for Word 88 validity before using Word 88 to
determine UDMA mask.  Note that the original xfer mask implementation
using ata_get_mode_mask() didn't consider bit 2 of Word 53.  This
patch introduces different (correct) behavior.

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

---

 libata-core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 42d43b5..e2529e9 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -819,7 +819,10 @@ static unsigned int ata_id_xfermask(cons
 	}
 
 	mwdma_mask = id[ATA_ID_MWDMA_MODES] & 0x07;
-	udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;
+
+	udma_mask = 0;
+	if (id[ATA_ID_FIELD_VALID] & (1 << 2))
+		udma_mask = id[ATA_ID_UDMA_MODES] & 0xff;
 
 	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
 }

[PATCH 3/6] libata: use ata_id_xfermask() in ata_dev_configure()

[Tejun Heo]

Replace quick & dirty max transfer mode determination in
ata_dev_configure() with ata_id_xfermask().  While at it, rename
xfer_modes variable to xfer_mask and make it unsigned int for
consistency.

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

---

 drivers/scsi/libata-core.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

58efdc973c86dc4fc8c7e4b43874a462a777257c
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9946618..84a8550 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1224,7 +1224,7 @@ static inline u8 ata_dev_knobble(const s
 static int ata_dev_configure(struct ata_port *ap, struct ata_device *dev,
 			     int print_info)
 {
-	unsigned long xfer_modes;
+	unsigned int xfer_mask;
 	int i, rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1255,12 +1255,8 @@ static int ata_dev_configure(struct ata_
 		goto err_out_nosup;
 	}
 
-	/* quick-n-dirty find max transfer mode; for printk only */
-	xfer_modes = dev->id[ATA_ID_UDMA_MODES];
-	if (!xfer_modes)
-		xfer_modes = (dev->id[ATA_ID_MWDMA_MODES]) << ATA_SHIFT_MWDMA;
-	if (!xfer_modes)
-		xfer_modes = ata_pio_modes(dev);
+	/* find max transfer mode; for printk only */
+	xfer_mask = ata_id_xfermask(dev->id);
 
 	ata_dump_id(dev->id);
 
@@ -1284,7 +1280,7 @@ static int ata_dev_configure(struct ata_
 				       "max %s, %Lu sectors: %s\n",
 				       ap->id, dev->devno,
 				       ata_id_major_version(dev->id),
-				       ata_mode_string(xfer_modes),
+				       ata_mode_string(xfer_mask),
 				       (unsigned long long)dev->n_sectors,
 				       lba_desc);
 		} else {
@@ -1308,7 +1304,7 @@ static int ata_dev_configure(struct ata_
 				       "max %s, %Lu sectors: CHS %u/%u/%u\n",
 				       ap->id, dev->devno,
 				       ata_id_major_version(dev->id),
-				       ata_mode_string(xfer_modes),
+				       ata_mode_string(xfer_mask),
 				       (unsigned long long)dev->n_sectors,
 				       dev->cylinders, dev->heads, dev->sectors);
 		}
@@ -1329,7 +1325,7 @@ static int ata_dev_configure(struct ata_
 		/* print device info to dmesg */
 		if (print_info)
 			printk(KERN_INFO "ata%u: dev %u ATAPI, max %s\n",
-			       ap->id, dev->devno, ata_mode_string(xfer_modes));
+			       ap->id, dev->devno, ata_mode_string(xfer_mask));
 	}
 
 	ap->host->max_cmd_len = 0;
-- 
1.2.4

[PATCH 6/6] libata: kill unused xfer_mode functions

[Tejun Heo]

Preceding xfer_mask changes make the following functions unused.

  ata_pio_modes(), base_from_shift(), ata_pr_blacklisted(), fgb()

Kill them.  Also, as xfer_mode_str[] is now only used by
ata_mode_string(), move it into the function.

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

---

This patch is against upstream + port_task patchset.

 drivers/scsi/libata-core.c |  108 +++++++-------------------------------------
 1 files changed, 18 insertions(+), 90 deletions(-)

7d1819a2066d9286e0a89e11808f7b5be92409b5
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index d211f0b..5acb079 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -67,7 +67,6 @@ static void ata_set_mode(struct ata_port
 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 int fgb(u32 bitmap);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -331,25 +330,6 @@ static int ata_xfer_mode2shift(unsigned 
 	return -1;
 }
 
-static const char * const xfer_mode_str[] = {
-	"PIO0",
-	"PIO1",
-	"PIO2",
-	"PIO3",
-	"PIO4",
-	"MWDMA0",
-	"MWDMA1",
-	"MWDMA2",
-	"UDMA/16",
-	"UDMA/25",
-	"UDMA/33",
-	"UDMA/44",
-	"UDMA/66",
-	"UDMA/100",
-	"UDMA/133",
-	"UDMA7",
-};
-
 /**
  *	ata_mode_string - convert xfer_mask to string
  *	@xfer_mask: mask of bits supported; only highest bit counts.
@@ -364,9 +344,26 @@ static const char * const xfer_mode_str[
  *	Constant C string representing highest speed listed in
  *	@mode_mask, or the constant C string "<n/a>".
  */
-
 static const char *ata_mode_string(unsigned int xfer_mask)
 {
+	static const char * const xfer_mode_str[] = {
+		"PIO0",
+		"PIO1",
+		"PIO2",
+		"PIO3",
+		"PIO4",
+		"MWDMA0",
+		"MWDMA1",
+		"MWDMA2",
+		"UDMA/16",
+		"UDMA/25",
+		"UDMA/33",
+		"UDMA/44",
+		"UDMA/66",
+		"UDMA/100",
+		"UDMA/133",
+		"UDMA7",
+	};
 	int highbit;
 
 	highbit = fls(xfer_mask) - 1;
@@ -827,35 +824,6 @@ static unsigned int ata_id_xfermask(cons
 	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
 }
 
-/*
- *	Compute the PIO modes available for this device. This is not as
- *	trivial as it seems if we must consider early devices correctly.
- *
- *	FIXME: pre IDE drive timing (do we care ?). 
- */
-
-static unsigned int ata_pio_modes(const struct ata_device *adev)
-{
-	u16 modes;
-
-	/* Usual case. Word 53 indicates word 64 is valid */
-	if (adev->id[ATA_ID_FIELD_VALID] & (1 << 1)) {
-		modes = adev->id[ATA_ID_PIO_MODES] & 0x03;
-		modes <<= 3;
-		modes |= 0x7;
-		return modes;
-	}
-
-	/* If word 64 isn't valid then Word 51 high byte holds the PIO timing
-	   number for the maximum. Turn it into a mask and return it */
-	modes = (2 << ((adev->id[ATA_ID_OLD_PIO_MODES] >> 8) & 0xFF)) - 1 ;
-	return modes;
-	/* But wait.. there's more. Design your standards by committee and
-	   you too can get a free iordy field to process. However its the 
-	   speeds not the modes that are supported... Note drivers using the
-	   timing API will get this right anyway */
-}
-
 /**
  *	ata_port_queue_task - Queue port_task
  *	@ap: The ata_port to queue port_task for
@@ -1728,26 +1696,6 @@ int ata_timing_compute(struct ata_device
 	return 0;
 }
 
-static const struct {
-	unsigned int shift;
-	u8 base;
-} xfer_mode_classes[] = {
-	{ ATA_SHIFT_UDMA,	XFER_UDMA_0 },
-	{ ATA_SHIFT_MWDMA,	XFER_MW_DMA_0 },
-	{ ATA_SHIFT_PIO,	XFER_PIO_0 },
-};
-
-static u8 base_from_shift(unsigned int shift)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(xfer_mode_classes); i++)
-		if (xfer_mode_classes[i].shift == shift)
-			return xfer_mode_classes[i].base;
-
-	return 0xff;
-}
-
 static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
 {
 	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
@@ -2596,13 +2544,6 @@ int ata_dev_revalidate(struct ata_port *
 	return rc;
 }
 
-static void ata_pr_blacklisted(const struct ata_port *ap,
-			       const struct ata_device *dev)
-{
-	printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, disabling DMA\n",
-		ap->id, dev->devno);
-}
-
 static const char * const ata_dma_blacklist [] = {
 	"WDC AC11000H",
 	"WDC AC22100H",
@@ -2690,19 +2631,6 @@ static unsigned int ata_dev_xfermask(str
 	return xfer_mask;
 }
 
-/* find greatest bit */
-static int fgb(u32 bitmap)
-{
-	unsigned int i;
-	int x = -1;
-
-	for (i = 0; i < 32; i++)
-		if (bitmap & (1 << i))
-			x = i;
-
-	return x;
-}
-
 /**
  *	ata_dev_set_xfermode - Issue SET FEATURES - XFER MODE command
  *	@ap: Port associated with device @dev
-- 
1.2.4

[PATCH 6/6] libata: kill unused xfer_mode functions

[Tejun Heo]

Preceding xfer_mask changes make the following functions unused.

  ata_pio_modes(), base_from_shift(), ata_pr_blacklisted(), fgb()

Kill them.  Also, as xfer_mode_str[] is now only used by
ata_mode_string(), move it into the function.

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

---

This patch is against upstream.

 drivers/scsi/libata-core.c |  108 +++++++-------------------------------------
 1 files changed, 18 insertions(+), 90 deletions(-)

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-03-06 04:14:11.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-03-06 04:14:30.000000000 +0900
@@ -67,7 +67,6 @@ static void ata_set_mode(struct ata_port
 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 int fgb(u32 bitmap);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -331,25 +330,6 @@ static int ata_xfer_mode2shift(unsigned 
 	return -1;
 }
 
-static const char * const xfer_mode_str[] = {
-	"PIO0",
-	"PIO1",
-	"PIO2",
-	"PIO3",
-	"PIO4",
-	"MWDMA0",
-	"MWDMA1",
-	"MWDMA2",
-	"UDMA/16",
-	"UDMA/25",
-	"UDMA/33",
-	"UDMA/44",
-	"UDMA/66",
-	"UDMA/100",
-	"UDMA/133",
-	"UDMA7",
-};
-
 /**
  *	ata_mode_string - convert xfer_mask to string
  *	@xfer_mask: mask of bits supported; only highest bit counts.
@@ -364,9 +344,26 @@ static const char * const xfer_mode_str[
  *	Constant C string representing highest speed listed in
  *	@mode_mask, or the constant C string "<n/a>".
  */
-
 static const char *ata_mode_string(unsigned int xfer_mask)
 {
+	static const char * const xfer_mode_str[] = {
+		"PIO0",
+		"PIO1",
+		"PIO2",
+		"PIO3",
+		"PIO4",
+		"MWDMA0",
+		"MWDMA1",
+		"MWDMA2",
+		"UDMA/16",
+		"UDMA/25",
+		"UDMA/33",
+		"UDMA/44",
+		"UDMA/66",
+		"UDMA/100",
+		"UDMA/133",
+		"UDMA7",
+	};
 	int highbit;
 
 	highbit = fls(xfer_mask) - 1;
@@ -827,35 +824,6 @@ static unsigned int ata_id_xfermask(cons
 	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
 }
 
-/*
- *	Compute the PIO modes available for this device. This is not as
- *	trivial as it seems if we must consider early devices correctly.
- *
- *	FIXME: pre IDE drive timing (do we care ?). 
- */
-
-static unsigned int ata_pio_modes(const struct ata_device *adev)
-{
-	u16 modes;
-
-	/* Usual case. Word 53 indicates word 64 is valid */
-	if (adev->id[ATA_ID_FIELD_VALID] & (1 << 1)) {
-		modes = adev->id[ATA_ID_PIO_MODES] & 0x03;
-		modes <<= 3;
-		modes |= 0x7;
-		return modes;
-	}
-
-	/* If word 64 isn't valid then Word 51 high byte holds the PIO timing
-	   number for the maximum. Turn it into a mask and return it */
-	modes = (2 << ((adev->id[ATA_ID_OLD_PIO_MODES] >> 8) & 0xFF)) - 1 ;
-	return modes;
-	/* But wait.. there's more. Design your standards by committee and
-	   you too can get a free iordy field to process. However its the 
-	   speeds not the modes that are supported... Note drivers using the
-	   timing API will get this right anyway */
-}
-
 static inline void
 ata_queue_packet_task(struct ata_port *ap)
 {
@@ -1718,26 +1686,6 @@ int ata_timing_compute(struct ata_device
 	return 0;
 }
 
-static const struct {
-	unsigned int shift;
-	u8 base;
-} xfer_mode_classes[] = {
-	{ ATA_SHIFT_UDMA,	XFER_UDMA_0 },
-	{ ATA_SHIFT_MWDMA,	XFER_MW_DMA_0 },
-	{ ATA_SHIFT_PIO,	XFER_PIO_0 },
-};
-
-static u8 base_from_shift(unsigned int shift)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(xfer_mode_classes); i++)
-		if (xfer_mode_classes[i].shift == shift)
-			return xfer_mode_classes[i].base;
-
-	return 0xff;
-}
-
 static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
 {
 	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
@@ -2586,13 +2534,6 @@ int ata_dev_revalidate(struct ata_port *
 	return rc;
 }
 
-static void ata_pr_blacklisted(const struct ata_port *ap,
-			       const struct ata_device *dev)
-{
-	printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, disabling DMA\n",
-		ap->id, dev->devno);
-}
-
 static const char * const ata_dma_blacklist [] = {
 	"WDC AC11000H",
 	"WDC AC22100H",
@@ -2680,19 +2621,6 @@ static unsigned int ata_dev_xfermask(str
 	return xfer_mask;
 }
 
-/* find greatest bit */
-static int fgb(u32 bitmap)
-{
-	unsigned int i;
-	int x = -1;
-
-	for (i = 0; i < 32; i++)
-		if (bitmap & (1 << i))
-			x = i;
-
-	return x;
-}
-
 /**
  *	ata_dev_set_xfermode - Issue SET FEATURES - XFER MODE command
  *	@ap: Port associated with device @dev

[PATCH 4/6] libata: use xfer_mask helpers in ata_dev_set_mode()

[Tejun Heo]

Rewrite hardcoded xfer_mode string determination in ata_dev_set_mode()
using xfer_mask helpers.

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

---

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

6c1926e74a4b5266dbcbb4156dee075a05bcc36f
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 84a8550..8849512 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1752,9 +1752,6 @@ static u8 base_from_shift(unsigned int s
 
 static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
 {
-	int ofs, idx;
-	u8 base;
-
 	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
 		return;
 
@@ -1763,22 +1760,18 @@ static void ata_dev_set_mode(struct ata_
 
 	ata_dev_set_xfermode(ap, dev);
 
-	base = base_from_shift(dev->xfer_shift);
-	ofs = dev->xfer_mode - base;
-	idx = ofs + dev->xfer_shift;
-	WARN_ON(idx >= ARRAY_SIZE(xfer_mode_str));
-
 	if (ata_dev_revalidate(ap, dev, 0)) {
 		printk(KERN_ERR "ata%u: failed to revalidate after set "
 		       "xfermode, disabled\n", ap->id);
 		ata_port_disable(ap);
 	}
 
-	DPRINTK("idx=%d xfer_shift=%u, xfer_mode=0x%x, base=0x%x, offset=%d\n",
-		idx, dev->xfer_shift, (int)dev->xfer_mode, (int)base, ofs);
+	DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
+		dev->xfer_shift, (int)dev->xfer_mode);
 
 	printk(KERN_INFO "ata%u: dev %u configured for %s\n",
-		ap->id, dev->devno, xfer_mode_str[idx]);
+	       ap->id, dev->devno,
+	       ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)));
 }
 
 static int ata_host_set_pio(struct ata_port *ap)
-- 
1.2.4

[PATCH 5/6] libata: reimplement ata_set_mode() using xfer_mask helpers

[Tejun Heo]

Use xfer_mask helpers to determine transfer mode.  This rewrite also
makes transfer mode determination done before any actual
configuration.  This patch doesn't result in any functional changes.

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

---

 drivers/scsi/libata-core.c |  233 +++++++++++++++-----------------------------
 1 files changed, 79 insertions(+), 154 deletions(-)

d06b4f9f33fd7a5ffbb93837f5b4a69d4fa91dbc
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8849512..d211f0b 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -65,11 +65,9 @@ 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_get_mode_mask(const struct ata_port *ap, int shift);
+static unsigned int ata_dev_xfermask(struct ata_port *ap,
+				     struct ata_device *dev);
 static int fgb(u32 bitmap);
-static int ata_choose_xfer_mode(const struct ata_port *ap,
-				u8 *xfer_mode_out,
-				unsigned int *xfer_shift_out);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -1776,51 +1774,42 @@ static void ata_dev_set_mode(struct ata_
 
 static int ata_host_set_pio(struct ata_port *ap)
 {
-	unsigned int mask;
-	int x, i;
-	u8 base, xfer_mode;
-
-	mask = ata_get_mode_mask(ap, ATA_SHIFT_PIO);
-	x = fgb(mask);
-	if (x < 0) {
-		printk(KERN_WARNING "ata%u: no PIO support\n", ap->id);
-		return -1;
-	}
-
-	base = base_from_shift(ATA_SHIFT_PIO);
-	xfer_mode = base + x;
-
-	DPRINTK("base 0x%x xfer_mode 0x%x mask 0x%x x %d\n",
-		(int)base, (int)xfer_mode, mask, x);
+	int i;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
-		if (ata_dev_present(dev)) {
-			dev->pio_mode = xfer_mode;
-			dev->xfer_mode = xfer_mode;
-			dev->xfer_shift = ATA_SHIFT_PIO;
-			if (ap->ops->set_piomode)
-				ap->ops->set_piomode(ap, dev);
+
+		if (!ata_dev_present(dev))
+			continue;
+
+		if (!dev->pio_mode) {
+			printk(KERN_WARNING "ata%u: no PIO support\n", ap->id);
+			return -1;
 		}
+
+		dev->xfer_mode = dev->pio_mode;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		if (ap->ops->set_piomode)
+			ap->ops->set_piomode(ap, dev);
 	}
 
 	return 0;
 }
 
-static void ata_host_set_dma(struct ata_port *ap, u8 xfer_mode,
-			    unsigned int xfer_shift)
+static void ata_host_set_dma(struct ata_port *ap)
 {
 	int i;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
-		if (ata_dev_present(dev)) {
-			dev->dma_mode = xfer_mode;
-			dev->xfer_mode = xfer_mode;
-			dev->xfer_shift = xfer_shift;
-			if (ap->ops->set_dmamode)
-				ap->ops->set_dmamode(ap, dev);
-		}
+
+		if (!ata_dev_present(dev) || !dev->dma_mode)
+			continue;
+
+		dev->xfer_mode = dev->dma_mode;
+		dev->xfer_shift = ata_xfer_mode2shift(dev->dma_mode);
+		if (ap->ops->set_dmamode)
+			ap->ops->set_dmamode(ap, dev);
 	}
 }
 
@@ -1835,28 +1824,34 @@ static void ata_host_set_dma(struct ata_
  */
 static void ata_set_mode(struct ata_port *ap)
 {
-	unsigned int xfer_shift;
-	u8 xfer_mode;
-	int rc;
+	int i, rc;
 
-	/* step 1: always set host PIO timings */
-	rc = ata_host_set_pio(ap);
-	if (rc)
-		goto err_out;
+	/* step 1: calculate xfer_mask */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
+		unsigned int xfer_mask;
+
+		if (!ata_dev_present(dev))
+			continue;
 
-	/* step 2: choose the best data xfer mode */
-	xfer_mode = xfer_shift = 0;
-	rc = ata_choose_xfer_mode(ap, &xfer_mode, &xfer_shift);
+		xfer_mask = 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));
+	}
+
+	/* step 2: always set host PIO timings */
+	rc = ata_host_set_pio(ap);
 	if (rc)
 		goto err_out;
 
-	/* step 3: if that xfer mode isn't PIO, set host DMA timings */
-	if (xfer_shift != ATA_SHIFT_PIO)
-		ata_host_set_dma(ap, xfer_mode, xfer_shift);
+	/* step 3: set host DMA timings */
+	ata_host_set_dma(ap);
 
 	/* step 4: update devices' xfer mode */
-	ata_dev_set_mode(ap, &ap->device[0]);
-	ata_dev_set_mode(ap, &ap->device[1]);
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		ata_dev_set_mode(ap, &ap->device[i]);
 
 	if (ap->flags & ATA_FLAG_PORT_DISABLED)
 		return;
@@ -2654,77 +2649,45 @@ static int ata_dma_blacklisted(const str
 	return 0;
 }
 
-static unsigned int ata_get_mode_mask(const struct ata_port *ap, int shift)
+/**
+ *	ata_dev_xfermask - Compute supported xfermask of the given device
+ *	@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...
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Computed xfermask.
+ */
+static unsigned int ata_dev_xfermask(struct ata_port *ap,
+				     struct ata_device *dev)
 {
-	const struct ata_device *master, *slave;
-	unsigned int mask;
+	unsigned long xfer_mask;
+	int i;
 
-	master = &ap->device[0];
-	slave = &ap->device[1];
+	xfer_mask = ata_pack_xfermask(ap->pio_mask, ap->mwdma_mask,
+				      ap->udma_mask);
 
-	WARN_ON(!ata_dev_present(master) && !ata_dev_present(slave));
-
-	if (shift == ATA_SHIFT_UDMA) {
-		mask = ap->udma_mask;
-		if (ata_dev_present(master)) {
-			mask &= (master->id[ATA_ID_UDMA_MODES] & 0xff);
-			if (ata_dma_blacklisted(master)) {
-				mask = 0;
-				ata_pr_blacklisted(ap, master);
-			}
-		}
-		if (ata_dev_present(slave)) {
-			mask &= (slave->id[ATA_ID_UDMA_MODES] & 0xff);
-			if (ata_dma_blacklisted(slave)) {
-				mask = 0;
-				ata_pr_blacklisted(ap, slave);
-			}
-		}
-	}
-	else if (shift == ATA_SHIFT_MWDMA) {
-		mask = ap->mwdma_mask;
-		if (ata_dev_present(master)) {
-			mask &= (master->id[ATA_ID_MWDMA_MODES] & 0x07);
-			if (ata_dma_blacklisted(master)) {
-				mask = 0;
-				ata_pr_blacklisted(ap, master);
-			}
-		}
-		if (ata_dev_present(slave)) {
-			mask &= (slave->id[ATA_ID_MWDMA_MODES] & 0x07);
-			if (ata_dma_blacklisted(slave)) {
-				mask = 0;
-				ata_pr_blacklisted(ap, slave);
-			}
-		}
-	}
-	else if (shift == ATA_SHIFT_PIO) {
-		mask = ap->pio_mask;
-		if (ata_dev_present(master)) {
-			/* spec doesn't return explicit support for
-			 * PIO0-2, so we fake it
-			 */
-			u16 tmp_mode = master->id[ATA_ID_PIO_MODES] & 0x03;
-			tmp_mode <<= 3;
-			tmp_mode |= 0x7;
-			mask &= tmp_mode;
-		}
-		if (ata_dev_present(slave)) {
-			/* spec doesn't return explicit support for
-			 * PIO0-2, so we fake it
-			 */
-			u16 tmp_mode = slave->id[ATA_ID_PIO_MODES] & 0x03;
-			tmp_mode <<= 3;
-			tmp_mode |= 0x7;
-			mask &= tmp_mode;
-		}
-	}
-	else {
-		mask = 0xffffffff; /* shut up compiler warning */
-		BUG();
+	/* use port-wide xfermask for now */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *d = &ap->device[i];
+		if (!ata_dev_present(d))
+			continue;
+		xfer_mask &= ata_id_xfermask(d->id);
+		if (ata_dma_blacklisted(d))
+			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
 	}
 
-	return mask;
+	if (ata_dma_blacklisted(dev))
+		printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, "
+		       "disabling DMA\n", ap->id, dev->devno);
+
+	return xfer_mask;
 }
 
 /* find greatest bit */
@@ -2741,44 +2704,6 @@ static int fgb(u32 bitmap)
 }
 
 /**
- *	ata_choose_xfer_mode - attempt to find best transfer mode
- *	@ap: Port for which an xfer mode will be selected
- *	@xfer_mode_out: (output) SET FEATURES - XFER MODE code
- *	@xfer_shift_out: (output) bit shift that selects this mode
- *
- *	Based on host and device capabilities, determine the
- *	maximum transfer mode that is amenable to all.
- *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
- *
- *	RETURNS:
- *	Zero on success, negative on error.
- */
-
-static int ata_choose_xfer_mode(const struct ata_port *ap,
-				u8 *xfer_mode_out,
-				unsigned int *xfer_shift_out)
-{
-	unsigned int mask, shift;
-	int x, i;
-
-	for (i = 0; i < ARRAY_SIZE(xfer_mode_classes); i++) {
-		shift = xfer_mode_classes[i].shift;
-		mask = ata_get_mode_mask(ap, shift);
-
-		x = fgb(mask);
-		if (x >= 0) {
-			*xfer_mode_out = xfer_mode_classes[i].base + x;
-			*xfer_shift_out = shift;
-			return 0;
-		}
-	}
-
-	return -1;
-}
-
-/**
  *	ata_dev_set_xfermode - Issue SET FEATURES - XFER MODE command
  *	@ap: Port associated with device @dev
  *	@dev: Device to which command will be sent
-- 
1.2.4