[PATCH 2.6.22-rc7 1/3] libata: clean up horkage handling

[PATCH 2.6.22-rc7 1/3] libata: clean up horkage handling

[Tejun Heo]

Horkage handling had the following problems.

* dev->horkage was positioned after ATA_DEVICE_CLEAR_OFFSET, so it was
  cleared before the device is configured.  This broke
  HORKAGE_DIAGNOSTIC.

* Some used dev->horkage while others called ata_device_blacklisted()
  directly.  This was at best confusing.

This patch moves dev->horkage right after dev->flags and set the field
according to the blacklist during device configuration.  All users
test against dev->horkage.  ata_device_blacklisted() now has only one
user, make it static.  While at it, rename it to ata_dev_blacklisted()
for consistency.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   13 ++++++++-----
 include/linux/libata.h    |    3 +--
 2 files changed, 9 insertions(+), 7 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -71,6 +71,7 @@ static unsigned int ata_dev_init_params(
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
+static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
 
 unsigned int ata_print_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -1814,7 +1815,7 @@ static void ata_dev_config_ncq(struct at
 		desc[0] = '\0';
 		return;
 	}
-	if (ata_device_blacklisted(dev) & ATA_HORKAGE_NONCQ) {
+	if (dev->horkage & ATA_HORKAGE_NONCQ) {
 		snprintf(desc, desc_sz, "NCQ (not used)");
 		return;
 	}
@@ -1862,6 +1863,9 @@ int ata_dev_configure(struct ata_device 
 	if (ata_msg_probe(ap))
 		ata_dev_printk(dev, KERN_DEBUG, "%s: ENTER\n", __FUNCTION__);
 
+	/* set horkage */
+	dev->horkage |= ata_dev_blacklisted(dev);
+
 	/* set _SDD */
 	rc = ata_acpi_push_id(dev);
 	if (rc) {
@@ -2042,7 +2046,7 @@ int ata_dev_configure(struct ata_device 
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
 
-	if (ata_device_blacklisted(dev) & ATA_HORKAGE_MAX_SEC_128)
+	if (dev->horkage & ATA_HORKAGE_MAX_SEC_128)
 		dev->max_sectors = min_t(unsigned int, ATA_MAX_SECTORS_128,
 					 dev->max_sectors);
 
@@ -3809,7 +3813,7 @@ static const struct ata_blacklist_entry 
 	{ }
 };
 
-unsigned long ata_device_blacklisted(const struct ata_device *dev)
+static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];
 	unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
@@ -3839,7 +3843,7 @@ static int ata_dma_blacklisted(const str
 	if ((dev->ap->flags & ATA_FLAG_PIO_POLLING) &&
 	    (dev->flags & ATA_DFLAG_CDB_INTR))
 		return 1;
-	return (ata_device_blacklisted(dev) & ATA_HORKAGE_NODMA) ? 1 : 0;
+	return (dev->horkage & ATA_HORKAGE_NODMA) ? 1 : 0;
 }
 
 /**
@@ -6875,7 +6879,6 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
 EXPORT_SYMBOL_GPL(ata_id_string);
 EXPORT_SYMBOL_GPL(ata_id_c_string);
 EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
-EXPORT_SYMBOL_GPL(ata_device_blacklisted);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
 
 EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -428,6 +428,7 @@ struct ata_device {
 	struct ata_port		*ap;
 	unsigned int		devno;		/* 0 or 1 */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
+	unsigned int		horkage;	/* List of broken features */
 	struct scsi_device	*sdev;		/* attached SCSI device */
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
@@ -456,7 +457,6 @@ struct ata_device {
 	/* error history */
 	struct ata_ering	ering;
 	int			spdn_cnt;
-	unsigned int		horkage;	/* List of broken features */
 #ifdef CONFIG_ATA_ACPI
 	/* ACPI objects info */
 	acpi_handle obj_handle;
@@ -772,7 +772,6 @@ extern void ata_id_string(const u16 *id,
 extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
 extern void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown);
-extern unsigned long ata_device_blacklisted(const struct ata_device *dev);
 extern void ata_bmdma_setup (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start (struct ata_queued_cmd *qc);
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);

[PATCH 2.6.22-rc7 2/3] libata: implement HORKAGE_SETXFER

[Tejun Heo]

Make CFA SETXFER failure handling into HORKAGE.  This will be used to
allow other cases to ignore SETXFER failure.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   15 ++++++++++-----
 include/linux/libata.h    |    1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -1898,7 +1898,7 @@ int ata_dev_configure(struct ata_device 
 	 * common ATA, ATAPI feature tests
 	 */
 
-	/* find max transfer mode; for printk only */
+	/* determine xfermask */
 	xfer_mask = ata_id_xfermask(id);
 
 	if (ata_msg_probe(ap))
@@ -1919,8 +1919,13 @@ int ata_dev_configure(struct ata_device 
 					       "supports DRM functions and may "
 					       "not be fully accessable.\n");
 			snprintf(revbuf, 7, "CFA");
-		}
-		else
+
+			/* Old CFA may refuse SETFEATURES_XFER, which
+			 * is just fine
+			 */
+			if (!(xfer_mask & ~ATA_MASK_PIO))
+				dev->horkage |= ATA_HORKAGE_SETXFER;
+		} else
 			snprintf(revbuf, 7, "ATA-%d",  ata_id_major_version(id));
 
 		dev->n_sectors = ata_id_n_sectors(id);
@@ -2775,8 +2780,8 @@ static int ata_dev_set_mode(struct ata_d
 		dev->flags |= ATA_DFLAG_PIO;
 
 	err_mask = ata_dev_set_xfermode(dev);
-	/* Old CFA may refuse this command, which is just fine */
-	if (dev->xfer_shift == ATA_SHIFT_PIO && ata_id_is_cfa(dev->id))
+
+	if (dev->horkage & ATA_HORKAGE_SETXFER)
         	err_mask &= ~AC_ERR_DEV;
 
 	if (err_mask) {
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -298,6 +298,7 @@ enum {
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
+	ATA_HORKAGE_SETXFER	= (1 << 4), /* SETFEATURES_XFER may fail */
 };
 
 enum hsm_task_states {

[PATCH 2.6.22-rc7 3/3] libata: pata_via CX700 quirks

[Tejun Heo]

For some reason, via decided that combined SATA/PATA setup in sata_via
isn't good enough and added a SATA port without SCR access to CX700.
For yet another unknown reason, slave slot of the SATA port dislikes
SETXFER.

This patch implements two quirks for CX700 - VIA_SATA_PATA and
VIA_BAD_SLAVE_SETXFER.  SATA_PATA indicates that the first port is
SATA and BAD_SLAVE_SETXFER indicates slave port may fail SETXFER.

  http://bugzilla.kernel.org/show_bug.cgi?id=8563

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/pata_via.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Index: work/drivers/ata/pata_via.c
===================================================================
--- work.orig/drivers/ata/pata_via.c
+++ work/drivers/ata/pata_via.c
@@ -84,6 +84,8 @@ enum {
 	VIA_BAD_ID	= 0x100, /* Has wrong vendor ID (0x1107) */
 	VIA_BAD_AST	= 0x200, /* Don't touch Address Setup Timing */
 	VIA_NO_ENABLES	= 0x400, /* Has no enablebits */
+	VIA_BAD_SLAVE_SETXFER = 0x0800, /* SETXFER on slave device may fail */
+	VIA_SATA_PATA	= 0x1000, /* SATA/PATA combined configuration */
 };
 
 /*
@@ -99,7 +101,7 @@ static const struct via_isa_bridge {
 } via_isa_bridges[] = {
 	{ "vt8237s",	PCI_DEVICE_ID_VIA_8237S,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "vt8251",	PCI_DEVICE_ID_VIA_8251,     0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
-	{ "cx700",	PCI_DEVICE_ID_VIA_CX700,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
+	{ "cx700",	PCI_DEVICE_ID_VIA_CX700,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST | VIA_SATA_PATA | VIA_BAD_SLAVE_SETXFER },
 	{ "vt6410",	PCI_DEVICE_ID_VIA_6410,     0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST | VIA_NO_ENABLES},
 	{ "vt8237a",	PCI_DEVICE_ID_VIA_8237A,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "vt8237",	PCI_DEVICE_ID_VIA_8237,     0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
@@ -168,6 +170,9 @@ static int via_cable_detect(struct ata_p
 	if (via_cable_override(pdev))
 		return ATA_CBL_PATA40_SHORT;
 
+	if ((config->flags & VIA_SATA_PATA) && ap->port_no == 0)
+		return ATA_CBL_SATA;
+
 	/* Early chips are 40 wire */
 	if ((config->flags & VIA_UDMA) < VIA_UDMA_66)
 		return ATA_CBL_PATA40;
@@ -292,6 +297,15 @@ static void via_do_set_mode(struct ata_p
 		pci_write_config_byte(pdev, 0x50 + offset, ut);
 }
 
+static void via_dev_config(struct ata_device *dev)
+{
+	const struct via_isa_bridge *config = dev->ap->host->private_data;
+
+	/* some new VIA chips have problem with SETXFER on slave slot */
+	if (dev->devno && (config->flags & VIA_BAD_SLAVE_SETXFER))
+		dev->horkage |= ATA_HORKAGE_SETXFER;
+}
+
 static void via_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
 	const struct via_isa_bridge *config = ap->host->private_data;
@@ -334,6 +348,7 @@ static struct scsi_host_template via_sht
 
 static struct ata_port_operations via_port_ops = {
 	.port_disable	= ata_port_disable,
+	.dev_config	= via_dev_config,
 	.set_piomode	= via_set_piomode,
 	.set_dmamode	= via_set_dmamode,
 	.mode_filter	= ata_pci_default_filter,
@@ -370,6 +385,7 @@ static struct ata_port_operations via_po
 
 static struct ata_port_operations via_port_ops_noirq = {
 	.port_disable	= ata_port_disable,
+	.dev_config	= via_dev_config,
 	.set_piomode	= via_set_piomode,
 	.set_dmamode	= via_set_dmamode,
 	.mode_filter	= ata_pci_default_filter,

Re: [PATCH 2.6.22-rc7 3/3] libata: pata_via CX700 quirks

[Alan Cox]

On Thu, 5 Jul 2007 13:39:49 +0900
Tejun Heo <htejun@gmail.com> wrote:

> For some reason, via decided that combined SATA/PATA setup in sata_via
> isn't good enough and added a SATA port without SCR access to CX700.

(It seems to be an integrated PATA/SATA bridge of some sort)

> For yet another unknown reason, slave slot of the SATA port dislikes
> SETXFER.
> 
> This patch implements two quirks for CX700 - VIA_SATA_PATA and
> VIA_BAD_SLAVE_SETXFER.  SATA_PATA indicates that the first port is
> SATA and BAD_SLAVE_SETXFER indicates slave port may fail SETXFER.

This makes no sense. If the SETXFER failed how did the device get into
the mode it is in ? How will change down work ? Why does the old IDE
driver work ?

I think this needs a lot more explanation

NAK

Re: [PATCH 2.6.22-rc7 3/3] libata: pata_via CX700 quirks

[Tejun Heo]

Alan Cox wrote:
> This makes no sense. If the SETXFER failed how did the device get into
> the mode it is in ? How will change down work ? Why does the old IDE
> driver work ?

IDE ignores SETXFER failure and for SATA devices SETXFER doesn't really
mean anything (unless it's bridged).  In this case, IDE didn't seem to
be seeing SETXFER failures but the chip being quite weird (SETXFER
failure is associated with the specific device slot not the device
itself.  it's probably a bridge problem.) and from the vendor it's from,
I thought whatever works is okay.

> I think this needs a lot more explanation

Feel free to re-open bug 8563.  I'd be happy to help. :-)

Thanks.

-- 
tejun

Re: [PATCH 2.6.22-rc7 2/3] libata: implement HORKAGE_SETXFER

[Alan Cox]

On Thu, 5 Jul 2007 13:35:01 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Make CFA SETXFER failure handling into HORKAGE.  This will be used to
> allow other cases to ignore SETXFER failure.

NAK

> +			/* Old CFA may refuse SETFEATURES_XFER, which
> +			 * is just fine
> +			 */
> +			if (!(xfer_mask & ~ATA_MASK_PIO))
> +				dev->horkage |= ATA_HORKAGE_SETXFER;
> +		} else

This is specifically done for PIO. An XFER failure for non PIO modes is a
serious failure and happens in the real world sometimes.

> -	/* Old CFA may refuse this command, which is just fine */
> -	if (dev->xfer_shift == ATA_SHIFT_PIO && ata_id_is_cfa(dev->id))
> +
> +	if (dev->horkage & ATA_HORKAGE_SETXFER)
>          	err_mask &= ~AC_ERR_DEV;

Re: [PATCH 2.6.22-rc7 2/3] libata: implement HORKAGE_SETXFER

[Tejun Heo]

Alan Cox wrote:
> On Thu, 5 Jul 2007 13:35:01 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> Make CFA SETXFER failure handling into HORKAGE.  This will be used to
>> allow other cases to ignore SETXFER failure.
> 
> NAK
> 
>> +			/* Old CFA may refuse SETFEATURES_XFER, which
>> +			 * is just fine
>> +			 */
>> +			if (!(xfer_mask & ~ATA_MASK_PIO))
>> +				dev->horkage |= ATA_HORKAGE_SETXFER;
>> +		} else
> 
> This is specifically done for PIO. An XFER failure for non PIO modes is a
> serious failure and happens in the real world sometimes.

Hmmm... That's what "if (!(xfer_mask & ~ATA_MASK_PIO))" test was for.
Or is it not enough?

-- 
tejun

Re: [PATCH 2.6.22-rc7 2/3] libata: implement HORKAGE_SETXFER

[Alan Cox]

On Thu, 05 Jul 2007 21:22:44 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Alan Cox wrote:
> > On Thu, 5 Jul 2007 13:35:01 +0900
> > Tejun Heo <htejun@gmail.com> wrote:
> > 
> >> Make CFA SETXFER failure handling into HORKAGE.  This will be used to
> >> allow other cases to ignore SETXFER failure.
> > 
> > NAK
> > 
> >> +			/* Old CFA may refuse SETFEATURES_XFER, which
> >> +			 * is just fine
> >> +			 */
> >> +			if (!(xfer_mask & ~ATA_MASK_PIO))
> >> +				dev->horkage |= ATA_HORKAGE_SETXFER;
> >> +		} else
> > 
> > This is specifically done for PIO. An XFER failure for non PIO modes is a
> > serious failure and happens in the real world sometimes.
> 
> Hmmm... That's what "if (!(xfer_mask & ~ATA_MASK_PIO))" test was for.
> Or is it not enough?

Actually you are correct the test is sufficient because we can't end up
adding modes later.

Un-NAK ;)

Alan

Re: [PATCH 2.6.22-rc7 1/3] libata: clean up horkage handling

[Alan Cox]

On Thu, 5 Jul 2007 13:31:27 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Horkage handling had the following problems.
> 
> * dev->horkage was positioned after ATA_DEVICE_CLEAR_OFFSET, so it was
>   cleared before the device is configured.  This broke
>   HORKAGE_DIAGNOSTIC.
> 
> * Some used dev->horkage while others called ata_device_blacklisted()
>   directly.  This was at best confusing.
> 
> This patch moves dev->horkage right after dev->flags and set the field
> according to the blacklist during device configuration.  All users
> test against dev->horkage.  ata_device_blacklisted() now has only one
> user, make it static.  While at it, rename it to ata_dev_blacklisted()
> for consistency.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Nice clean up

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 2.6.22-rc7 1/3] libata: clean up horkage handling

[Jeff Garzik]

Tejun Heo wrote:
> Horkage handling had the following problems.
> 
> * dev->horkage was positioned after ATA_DEVICE_CLEAR_OFFSET, so it was
>   cleared before the device is configured.  This broke
>   HORKAGE_DIAGNOSTIC.
> 
> * Some used dev->horkage while others called ata_device_blacklisted()
>   directly.  This was at best confusing.
> 
> This patch moves dev->horkage right after dev->flags and set the field
> according to the blacklist during device configuration.  All users
> test against dev->horkage.  ata_device_blacklisted() now has only one
> user, make it static.  While at it, rename it to ata_dev_blacklisted()
> for consistency.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-core.c |   13 ++++++++-----
>  include/linux/libata.h    |    3 +--
>  2 files changed, 9 insertions(+), 7 deletions(-)

applied