[PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

currently libata-sff is completely ignoring the enabled/disabled status
of the interfaces.  This is a real problem on parisc because if you
touch a non responding memory area (i.e. a disabled interface) you crash
the box.

Fix this up by restoring the enablebits logic to the pata_cmd64x driver.
To do this, libata-sff has to be modified not to probe both ports if we
don't have them.  This is done by reintroducing IDE_HFLAG_SINGLE flag as
ATA_HOST_SFF_SINGLE_PORT which drivers can use to condition libata-sff
port probing.

James

---

James Bottomley (2):
  libata-sff: remove hardcoded requirement for two ports
  pata_cmd64x: fix crash on boot with disabled secondary port

 drivers/ata/libata-sff.c  |   75 ++++++++++++++++++++++++++++++++------------
 drivers/ata/pata_cmd64x.c |   22 +++++++++++--
 include/linux/libata.h    |    1 +
 3 files changed, 74 insertions(+), 24 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] libata-sff: remove hardcoded requirement for two ports

[James Bottomley]

The two port requirement in libata-sff is causing crashes on non-x86
systems which are wired up with the second port disabled.

Fix libata-sff to key of a new host flag ATA_HOST_SFF_SINGLE_PORT
which tells it only to probe the primary port of the host.  This
prevents the crash by preventing the disabled secondary registers from
being touched.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/ata/libata-sff.c |   75 +++++++++++++++++++++++++++++++++-------------
 include/linux/libata.h   |    1 +
 2 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index f8380ce..f0aa7db 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2296,7 +2296,7 @@ int ata_pci_sff_init_host(struct ata_host *host)
 	int i, rc;
 
 	/* request, iomap BARs and init port addresses accordingly */
-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 		int base = i * 2;
 		void __iomem * const *iomap;
@@ -2349,10 +2349,11 @@ int ata_pci_sff_init_host(struct ata_host *host)
 EXPORT_SYMBOL_GPL(ata_pci_sff_init_host);
 
 /**
- *	ata_pci_sff_prepare_host - helper to prepare PCI PIO-only SFF ATA host
+ *	ata_pci_sff_prepare_host_ports - helper to prepare PCI PIO-only SFF ATA host
  *	@pdev: target PCI device
  *	@ppi: array of port_info, must be enough for two ports
  *	@r_host: out argument for the initialized ATA host
+ *	@ports: number of ports in the host
  *
  *	Helper to allocate PIO-only SFF ATA host for @pdev, acquire
  *	all PCI resources and initialize it accordingly in one go.
@@ -2363,9 +2364,9 @@ EXPORT_SYMBOL_GPL(ata_pci_sff_init_host);
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int ata_pci_sff_prepare_host(struct pci_dev *pdev,
-			     const struct ata_port_info * const *ppi,
-			     struct ata_host **r_host)
+static int ata_pci_sff_prepare_host_ports(struct pci_dev *pdev,
+					  const struct ata_port_info * const *ppi,
+					  struct ata_host **r_host, int nports)
 {
 	struct ata_host *host;
 	int rc;
@@ -2373,7 +2374,7 @@ int ata_pci_sff_prepare_host(struct pci_dev *pdev,
 	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
-	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
+	host = ata_host_alloc_pinfo(&pdev->dev, ppi, nports);
 	if (!host) {
 		dev_printk(KERN_ERR, &pdev->dev,
 			   "failed to allocate ATA host\n");
@@ -2393,6 +2394,27 @@ err_out:
 	devres_release_group(&pdev->dev, NULL);
 	return rc;
 }
+
+/**
+ *	ata_pci_sff_prepare_host - helper to prepare PCI PIO-only SFF ATA host
+ *	@pdev: target PCI device
+ *	@ppi: array of port_info, must be enough for two ports
+ *	@r_host: out argument for the initialized ATA host
+ *
+ *	Helper to allocate PIO-only SFF ATA host for @pdev, acquire
+ *	all PCI resources and initialize it accordingly in one go.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_pci_sff_prepare_host(struct pci_dev *pdev,
+			     const struct ata_port_info * const *ppi,
+			     struct ata_host **r_host) {
+	return ata_pci_sff_prepare_host_ports(pdev, ppi, r_host, 2);
+}
 EXPORT_SYMBOL_GPL(ata_pci_sff_prepare_host);
 
 /**
@@ -2447,13 +2469,15 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 		return -ENOMEM;
 
 	if (!legacy_mode && pdev->irq) {
+		int i;
+
 		rc = devm_request_irq(dev, pdev->irq, irq_handler,
 				      IRQF_SHARED, drv_name, host);
 		if (rc)
 			goto out;
 
-		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
-		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
+		for (i = 0; i < host->n_ports; i++)
+			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
 	} else if (legacy_mode) {
 		if (!ata_port_is_dummy(host->ports[0])) {
 			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
@@ -2466,7 +2490,7 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 				      ATA_PRIMARY_IRQ(pdev));
 		}
 
-		if (!ata_port_is_dummy(host->ports[1])) {
+		if (host->n_ports > 1 && !ata_port_is_dummy(host->ports[1])) {
 			rc = devm_request_irq(dev, ATA_SECONDARY_IRQ(pdev),
 					      irq_handler, IRQF_SHARED,
 					      drv_name, host);
@@ -2532,6 +2556,7 @@ int ata_pci_sff_init_one(struct pci_dev *pdev,
 	const struct ata_port_info *pi;
 	struct ata_host *host = NULL;
 	int rc;
+	int nports = (hflag & ATA_HOST_SFF_SINGLE_PORT) ? 1 : 2;
 
 	DPRINTK("ENTER\n");
 
@@ -2550,7 +2575,7 @@ int ata_pci_sff_init_one(struct pci_dev *pdev,
 		goto out;
 
 	/* prepare and activate SFF host */
-	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
+	rc = ata_pci_sff_prepare_host_ports(pdev, ppi, &host, nports);
 	if (rc)
 		goto out;
 	host->private_data = host_priv;
@@ -3162,7 +3187,7 @@ static void ata_bmdma_nodma(struct ata_host *host, const char *reason)
 	dev_printk(KERN_ERR, host->dev, "BMDMA: %s, falling back to PIO\n",
 		   reason);
 
-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < host->n_ports; i++) {
 		host->ports[i]->mwdma_mask = 0;
 		host->ports[i]->udma_mask = 0;
 	}
@@ -3213,7 +3238,7 @@ void ata_pci_bmdma_init(struct ata_host *host)
 	}
 	host->iomap = pcim_iomap_table(pdev);
 
-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 		void __iomem *bmdma = host->iomap[4] + 8 * i;
 
@@ -3231,6 +3256,20 @@ void ata_pci_bmdma_init(struct ata_host *host)
 }
 EXPORT_SYMBOL_GPL(ata_pci_bmdma_init);
 
+static int ata_pci_bmdma_prepare_host_ports(struct pci_dev *pdev,
+					    const struct ata_port_info * const * ppi,
+					    struct ata_host **r_host, int nports)
+{
+	int rc;
+
+	rc = ata_pci_sff_prepare_host_ports(pdev, ppi, r_host, nports);
+	if (rc)
+		return rc;
+
+	ata_pci_bmdma_init(*r_host);
+	return 0;
+}
+
 /**
  *	ata_pci_bmdma_prepare_host - helper to prepare PCI BMDMA ATA host
  *	@pdev: target PCI device
@@ -3250,14 +3289,7 @@ int ata_pci_bmdma_prepare_host(struct pci_dev *pdev,
 			       const struct ata_port_info * const * ppi,
 			       struct ata_host **r_host)
 {
-	int rc;
-
-	rc = ata_pci_sff_prepare_host(pdev, ppi, r_host);
-	if (rc)
-		return rc;
-
-	ata_pci_bmdma_init(*r_host);
-	return 0;
+	return ata_pci_bmdma_prepare_host_ports(pdev, ppi, r_host, 2);
 }
 EXPORT_SYMBOL_GPL(ata_pci_bmdma_prepare_host);
 
@@ -3287,6 +3319,7 @@ int ata_pci_bmdma_init_one(struct pci_dev *pdev,
 	const struct ata_port_info *pi;
 	struct ata_host *host = NULL;
 	int rc;
+	int nports = (hflags & ATA_HOST_SFF_SINGLE_PORT) ? 1 : 2;
 
 	DPRINTK("ENTER\n");
 
@@ -3305,7 +3338,7 @@ int ata_pci_bmdma_init_one(struct pci_dev *pdev,
 		goto out;
 
 	/* prepare and activate BMDMA host */
-	rc = ata_pci_bmdma_prepare_host(pdev, ppi, &host);
+	rc = ata_pci_bmdma_prepare_host_ports(pdev, ppi, &host, nports);
 	if (rc)
 		goto out;
 	host->private_data = host_priv;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7f675aa..554b0d2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -238,6 +238,7 @@ enum {
 	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host only */
 	ATA_HOST_STARTED	= (1 << 1),	/* Host started */
 	ATA_HOST_PARALLEL_SCAN	= (1 << 2),	/* Ports on this host can be scanned in parallel */
+	ATA_HOST_SFF_SINGLE_PORT = (1 << 3),	/* SFF interface should only probe one port not two */
 
 	/* bits 24:31 of host->flags are reserved for LLD specific flags */
 
-- 
1.7.4.1

[PATCH 2/2] pata_cmd64x: fix crash on boot with disabled secondary port

[James Bottomley]

On non-x86 systems, probing a port which is listed as disabled can
cause an immediate crash.  Fix the driver not to do this by porting
the enablebits check from the IDE driver and setting the
ATA_HOST_SFF_SINGLE_PORT flag if only the primary is enabled.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/ata/pata_cmd64x.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index 905ff76..aa71a13 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -41,6 +41,9 @@
 enum {
 	CFR 		= 0x50,
 		CFR_INTR_CH0  = 0x04,
+	ENPORT		= 0x51,
+		ENPORT_PRIMARY   = 0x04,
+		ENPORT_SECONDARY = 0x08,
 	CMDTIM 		= 0x52,
 	ARTTIM0 	= 0x53,
 	DRWTIM0 	= 0x54,
@@ -329,8 +332,8 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		}
 	};
 	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
-	u8 mrdmode;
-	int rc;
+	u8 mrdmode, reg;
+	int rc, hflags = 0;
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -354,6 +357,19 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	mrdmode |= 0x02;	/* Memory read line enable */
 	pci_write_config_byte(pdev, MRDMODE, mrdmode);
 
+	/* check for enabled ports */
+	pci_read_config_byte(pdev, ENPORT, &reg);
+	/* the cm643 primary port is always enabled */
+	if (id->driver_data != 0 && !(reg & ENPORT_PRIMARY)) {
+		dev_printk(KERN_ERR, &pdev->dev, "Primary port is disabled; detaching\n");
+		return -ENODEV;
+		
+	}
+	if (!(reg & ENPORT_SECONDARY)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n");
+		hflags |= ATA_HOST_SFF_SINGLE_PORT;
+	}
+
 	/* Force PIO 0 here.. */
 
 	/* PPC specific fixup copied from old driver */
@@ -361,7 +377,7 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	pci_write_config_byte(pdev, UDIDETCR0, 0xF0);
 #endif
 
-	return ata_pci_bmdma_init_one(pdev, ppi, &cmd64x_sht, NULL, 0);
+	return ata_pci_bmdma_init_one(pdev, ppi, &cmd64x_sht, NULL, hflags);
 }
 
 #ifdef CONFIG_PM
-- 
1.7.4.1

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

On Mon, 18 Apr 2011 13:42:27 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> currently libata-sff is completely ignoring the enabled/disabled status
> of the interfaces. 

Yes - it makes some machines work rather better because the BIOSes
sometimes mess up the registers. It wad a *deliberate* decision not to
port it over and as a result stuff works that failed before. Windows
drivers clearly ignore the bits in many cases.

In addition
- Your patch seems to be applying the enable bits apply in native mode
  (they don't generally)
- You seem to be assuming either first or both ports enabled, secondly
  only is just as valid

This matters for CMD64x as several 64x devices are found on hotpluggable
'docking stations' using a PCI split bridge. Only doing the checks for
chips in legacy mode should avoid that problem. In native mode the PCI
bars are the only register bases used so the problem doesn't arise.

The patch seems to be broken for all these cases and also incredibly
invasive given you can just pass a dummy port in as one of
your struct ata_port_info * pointers to ata_pci_dma_init_one()

You shouldn't need to touch a single line of the core libata code,
although it might be the best way of doing it. Either way if you do the
number of ports needs to be a bitmask instead and you need to leave
native mode alone.

Alan

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Mon, 2011-04-18 at 20:52 +0100, Alan Cox wrote:
> On Mon, 18 Apr 2011 13:42:27 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > currently libata-sff is completely ignoring the enabled/disabled status
> > of the interfaces. 
> 
> Yes - it makes some machines work rather better because the BIOSes
> sometimes mess up the registers. It wad a *deliberate* decision not to
> port it over and as a result stuff works that failed before. Windows
> drivers clearly ignore the bits in many cases.

Well your deliberate decision is crashing my box on boot.  That makes
this a regression from the IDE cmd64x driver. I hear indirectly there's
a similar problem on sparc.

> In addition
> - Your patch seems to be applying the enable bits apply in native mode
>   (they don't generally)
> - You seem to be assuming either first or both ports enabled, secondly
>   only is just as valid

Yes, I know ... but that config is broken today and a fix, given the
hardcoded assumptions in libata-sff, would be more invasive (and not
really of interest to the crash problem).

> This matters for CMD64x as several 64x devices are found on hotpluggable
> 'docking stations' using a PCI split bridge. Only doing the checks for
> chips in legacy mode should avoid that problem. In native mode the PCI
> bars are the only register bases used so the problem doesn't arise.

No, that analysis is wrong: Parisc (and actually presumably every
non-x86) doesn't use legacy mode.  The bars are all wired up (I posted
the original lspci output showing this).  The problem is that when you
try to prod the registers behind the secondary bar we get an instant
crash because the memory doesn't respond ... I'm assuming the secondary
bar isn't actually wired up to the chip.

> The patch seems to be broken for all these cases and also incredibly
> invasive given you can just pass a dummy port in as one of
> your struct ata_port_info * pointers to ata_pci_dma_init_one()

You mean in ata_pci_bmdma_init_one()?  yes, that might work ... I still
need to know how to detect this.

> You shouldn't need to touch a single line of the core libata code,
> although it might be the best way of doing it. Either way if you do the
> number of ports needs to be a bitmask instead and you need to leave
> native mode alone.

The core libata code looked broken in the assumption that it had to poke
a double register pair for sff mode (the hard coded loop over two
ports) ... this is what won't work on non-x86 boxes.  

James

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 18 Apr 2011 15:08:07 -0500

> On Mon, 2011-04-18 at 20:52 +0100, Alan Cox wrote:
>> On Mon, 18 Apr 2011 13:42:27 -0500
>> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>> 
>> > currently libata-sff is completely ignoring the enabled/disabled status
>> > of the interfaces. 
>> 
>> Yes - it makes some machines work rather better because the BIOSes
>> sometimes mess up the registers. It wad a *deliberate* decision not to
>> port it over and as a result stuff works that failed before. Windows
>> drivers clearly ignore the bits in many cases.
> 
> Well your deliberate decision is crashing my box on boot.  That makes
> this a regression from the IDE cmd64x driver. I hear indirectly there's
> a similar problem on sparc.

Yep, similar problems exist on sparc64, bus errors.

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Mon, 2011-04-18 at 20:52 +0100, Alan Cox wrote:
> You shouldn't need to touch a single line of the core libata code,
> although it might be the best way of doing it.

So how about this, using the dummy port info mechanism.  I get a
spurious ata2: DUMMY message, but I suppose libata people are used to
that.  I still have to fix libata to prevent spurious irq information,
but that's cosmetic.

James

---

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index f8380ce..b1b926c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 		return -ENOMEM;
 
 	if (!legacy_mode && pdev->irq) {
+		int i;
+
 		rc = devm_request_irq(dev, pdev->irq, irq_handler,
 				      IRQF_SHARED, drv_name, host);
 		if (rc)
 			goto out;
 
-		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
-		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
+		for (i = 0; i < 2; i++) {
+			if (ata_port_is_dummy(host->ports[i]))
+				continue;
+			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
+		}
 	} else if (legacy_mode) {
 		if (!ata_port_is_dummy(host->ports[0])) {
 			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index 905ff76..10dabe9 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -41,6 +41,9 @@
 enum {
 	CFR 		= 0x50,
 		CFR_INTR_CH0  = 0x04,
+	ENPORT		= 0x51,
+		ENPORT_PRIMARY   = 0x04,
+		ENPORT_SECONDARY = 0x08,
 	CMDTIM 		= 0x52,
 	ARTTIM0 	= 0x53,
 	DRWTIM0 	= 0x54,
@@ -328,8 +331,12 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 			.port_ops = &cmd648_port_ops
 		}
 	};
-	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
-	u8 mrdmode;
+	const struct ata_port_info *ppi[] = { 
+		&cmd_info[id->driver_data],
+		&cmd_info[id->driver_data],
+		NULL
+	};
+	u8 mrdmode, reg;
 	int rc;
 
 	rc = pcim_enable_device(pdev);
@@ -354,6 +361,19 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	mrdmode |= 0x02;	/* Memory read line enable */
 	pci_write_config_byte(pdev, MRDMODE, mrdmode);
 
+	/* check for enabled ports */
+	pci_read_config_byte(pdev, ENPORT, &reg);
+	/* the cm643 primary port is always enabled */
+	if (id->driver_data != 0 && !(reg & ENPORT_PRIMARY)) {
+		dev_printk(KERN_ERR, &pdev->dev, "Primary port is disabled; detaching\n");
+		ppi[0] = &ata_dummy_port_info;
+		
+	}
+	if (!(reg & ENPORT_SECONDARY)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n");
+		ppi[1] = &ata_dummy_port_info;
+	}
+
 	/* Force PIO 0 here.. */
 
 	/* PPC specific fixup copied from old driver */

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

> Well your deliberate decision is crashing my box on boot.  That makes
> this a regression from the IDE cmd64x driver. I hear indirectly there's
> a similar problem on sparc.

Sure - I'm not saying there isn't a problem just cautioning that there is
more to fixing it than blindly adding checks. If you simply check the
fields you break the split bridge boxes on x86. They aren't very common
these days but they probably outnumber PA-RISC Linux users 8)

> No, that analysis is wrong: Parisc (and actually presumably every
> non-x86) doesn't use legacy mode.  The bars are all wired up (I posted
> the original lspci output showing this).  The problem is that when you
> try to prod the registers behind the secondary bar we get an instant
> crash because the memory doesn't respond ... I'm assuming the secondary
> bar isn't actually wired up to the chip.

Then your PCI configuration is faulty - no ?

I'd have thought the best way to fix that would be a PCI quirk for the
platform. However if it's multiple non x86 platforms being hit then the
driver probably needs the smarts to cope with this weird wiring.

> > The patch seems to be broken for all these cases and also incredibly
> > invasive given you can just pass a dummy port in as one of
> > your struct ata_port_info * pointers to ata_pci_dma_init_one()
> 
> You mean in ata_pci_bmdma_init_one()?  yes, that might work ... I still
> need to know how to detect this.
> 
> > You shouldn't need to touch a single line of the core libata code,
> > although it might be the best way of doing it. Either way if you do the
> > number of ports needs to be a bitmask instead and you need to leave
> > native mode alone.
> 
> The core libata code looked broken in the assumption that it had to poke
> a double register pair for sff mode (the hard coded loop over two
> ports) ... this is what won't work on non-x86 boxes.  

Worked implementation example: drivers/ata/pata_via.c - see the use of
VIA_IDFLAG_SINGLE

I think that will do what you need ?

Alan

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

>	ata_port_desc(host->ports[i], "irq %d", pdev->irq);
> +		}
>  	} else if (legacy_mode) {
>  		if (!ata_port_is_dummy(host->ports[0])) {
>  			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),

This bit looks fine - in fact it's a nice clean up anyway

> +	}
> +	if (!(reg & ENPORT_SECONDARY)) {
> +		dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n");
> +		ppi[1] = &ata_dummy_port_info;
> +	}

And you just broke split bridge setups. Also need to check if the bits
are valid for this chipset in native mode officialy - Sergei probably
knows the answer to that.

We can detect the Mobility electronics split bridges at least (and I
suspect they are the only 'common' CMD64x hot plug device indeed possibly
the only one) because the parent bridge of the CMD64x will have a PCI
vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.


Alan

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Mon, 2011-04-18 at 22:20 +0100, Alan Cox wrote:
> We can detect the Mobility electronics split bridges at least (and I
> suspect they are the only 'common' CMD64x hot plug device indeed possibly
> the only one) because the parent bridge of the CMD64x will have a PCI
> vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.

Trying to wire in a bridge blacklist looks a bit dicey ... unless you're
sure this is (and will always be) the only bridge?  How should this
wiring be done? most of the bridge checks just see if the bridge is in
the system rather than walking up the device tree; is that OK?

If not, what about just a legacy check on X86, so hedge with

if (legacy || !CONFIG_X86)

(or even just dump the legacy check, since you think everything is fine
on x86 today?)

James

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

> If not, what about just a legacy check on X86, so hedge with
> 
> if (legacy || !CONFIG_X86)
> 
> (or even just dump the legacy check, since you think everything is fine
> on x86 today?)

Well in theory you can plug a cardbus one into a non x86 assuming the
firmware doesn't explode in protest anyway !

I don't think the bridge walk is particularly tricky - it will always be
the direct parent bridge

So it's a matter of

	struct pci_dev *bridge = dev->bus->self;
	if (bridge && bridge->vendor == 0x14f2)

Alan

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Tue, 2011-04-19 at 15:36 +0100, Alan Cox wrote:
> > If not, what about just a legacy check on X86, so hedge with
> > 
> > if (legacy || !CONFIG_X86)
> > 
> > (or even just dump the legacy check, since you think everything is fine
> > on x86 today?)
> 
> Well in theory you can plug a cardbus one into a non x86 assuming the
> firmware doesn't explode in protest anyway !
> 
> I don't think the bridge walk is particularly tricky - it will always be
> the direct parent bridge

OK, that's what I was looking for.  Most of the other bridge checks just
see if the bridge is present in the system.

> So it's a matter of
> 
> 	struct pci_dev *bridge = dev->bus->self;
> 	if (bridge && bridge->vendor == 0x14f2)

Which vendor is 0x14f2?  It probably should have a PCI_VENDOR_ID_...
define.

James

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

> > 	struct pci_dev *bridge = dev->bus->self;
> > 	if (bridge && bridge->vendor == 0x14f2)
> 
> Which vendor is 0x14f2?  It probably should have a PCI_VENDOR_ID_...
> define.

Jeff whines about those, Its Mobility Electronics.

Alan

Re: [PATCH 2/2] pata_cmd64x: fix crash on boot with disabled secondary port

[Sergei Shtylyov]

Hello.

On 18-04-2011 22:45, James Bottomley wrote:

> On non-x86 systems, probing a port which is listed as disabled can
> cause an immediate crash.  Fix the driver not to do this by porting
> the enablebits check from the IDE driver and setting the
> ATA_HOST_SFF_SINGLE_PORT flag if only the primary is enabled.

> Signed-off-by: James Bottomley<James.Bottomley@HansenPartnership.com>
> ---
>   drivers/ata/pata_cmd64x.c |   22 +++++++++++++++++++---
>   1 files changed, 19 insertions(+), 3 deletions(-)

> diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
> index 905ff76..aa71a13 100644
> --- a/drivers/ata/pata_cmd64x.c
> +++ b/drivers/ata/pata_cmd64x.c
> @@ -41,6 +41,9 @@
>   enum {
>   	CFR 		= 0x50,
>   		CFR_INTR_CH0  = 0x04,
> +	ENPORT		= 0x51,

    The register is actually called CNTRL.
    BTW, the PCI-649 specification is available at:
http://gkernel.sourceforge.net/specs/sii/SiI649-DS-0066.pdf.bz2

> +		ENPORT_PRIMARY   = 0x04,
> +		ENPORT_SECONDARY = 0x08,
>   	CMDTIM 		= 0x52,
>   	ARTTIM0 	= 0x53,
>   	DRWTIM0 	= 0x54,
> @@ -329,8 +332,8 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   		}
>   	};
>   	const struct ata_port_info *ppi[] = {&cmd_info[id->driver_data], NULL };
> -	u8 mrdmode;
> -	int rc;
> +	u8 mrdmode, reg;
> +	int rc, hflags = 0;
>
>   	rc = pcim_enable_device(pdev);
>   	if (rc)
> @@ -354,6 +357,19 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mrdmode |= 0x02;	/* Memory read line enable */
>   	pci_write_config_byte(pdev, MRDMODE, mrdmode);
>
> +	/* check for enabled ports */
> +	pci_read_config_byte(pdev, ENPORT, &reg);
> +	/* the cm643 primary port is always enabled */
> +	if (id->driver_data != 0&&  !(reg&  ENPORT_PRIMARY)) {

    That's not quite correct. The original PCI0646 didn't have the primary 
channel enable bit as well as PCI0643; only PCI0646U+ had the bit in question.

> +		dev_printk(KERN_ERR, &pdev->dev, "Primary port is disabled; detaching\n");
> +		return -ENODEV;

    Why? It's perfectly valid to have only secondary port enabled.

WBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

On 19-04-2011 0:50, James Bottomley wrote:

>> You shouldn't need to touch a single line of the core libata code,
>> although it might be the best way of doing it.

> So how about this, using the dummy port info mechanism.  I get a
> spurious ata2: DUMMY message, but I suppose libata people are used to
> that.  I still have to fix libata to prevent spurious irq information,
> but that's cosmetic.

> James

> ---

> diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
> index 905ff76..10dabe9 100644
> --- a/drivers/ata/pata_cmd64x.c
> +++ b/drivers/ata/pata_cmd64x.c
> @@ -354,6 +361,19 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mrdmode |= 0x02;	/* Memory read line enable */
>   	pci_write_config_byte(pdev, MRDMODE, mrdmode);
>
> +	/* check for enabled ports */
> +	pci_read_config_byte(pdev, ENPORT,&reg);
> +	/* the cm643 primary port is always enabled */
> +	if (id->driver_data != 0&&  !(reg&  ENPORT_PRIMARY)) {
> +		dev_printk(KERN_ERR,&pdev->dev, "Primary port is disabled; detaching\n");

    This is no longer true.

> +		ppi[0] =&ata_dummy_port_info;
> +		
> +	}

WBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

On 19-04-2011 1:20, Alan Cox wrote:

>> +	}
>> +	if (!(reg&  ENPORT_SECONDARY)) {
>> +		dev_printk(KERN_NOTICE,&pdev->dev, "Secondary port is disabled\n");
>> +		ppi[1] =&ata_dummy_port_info;
>> +	}

> And you just broke split bridge setups. Also need to check if the bits
> are valid for this chipset in native mode officialy - Sergei probably
> knows the answer to that.

    The PCI-649 spec. doesn't say anything about legacy/native mode WRT the 
channel enable bits.

> We can detect the Mobility electronics split bridges at least (and I
> suspect they are the only 'common' CMD64x hot plug device indeed possibly
> the only one) because the parent bridge of the CMD64x will have a PCI
> vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.

    What's the issue with these brodges anyway? Why the enable bits are not 
valid for them?

WBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

> > We can detect the Mobility electronics split bridges at least (and I
> > suspect they are the only 'common' CMD64x hot plug device indeed possibly
> > the only one) because the parent bridge of the CMD64x will have a PCI
> > vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.
> 
>     What's the issue with these brodges anyway? Why the enable bits are not 
> valid for them?

Good question. I'd assumed because they are hotplugged and so no firmware
gets to set the bits properly ?

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

On 20-04-2011 1:19, Alan Cox wrote:

>>> We can detect the Mobility electronics split bridges at least (and I
>>> suspect they are the only 'common' CMD64x hot plug device indeed possibly
>>> the only one) because the parent bridge of the CMD64x will have a PCI
>>> vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.

>>      What's the issue with these brodges anyway? Why the enable bits are not
>> valid for them?

> Good question. I'd assumed because they are hotplugged and so no firmware
> gets to set the bits properly ?

    The bits are strapped off the pins JP3/JP4.

WBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

On Wed, 20 Apr 2011 01:22:54 +0400
Sergei Shtylyov <sshtylyov@mvista.com> wrote:

> On 20-04-2011 1:19, Alan Cox wrote:
> 
> >>> We can detect the Mobility electronics split bridges at least (and I
> >>> suspect they are the only 'common' CMD64x hot plug device indeed possibly
> >>> the only one) because the parent bridge of the CMD64x will have a PCI
> >>> vendor id of 0x14f2 and a device id 0x0001, 0x0002, or 0x0120.
> 
> >>      What's the issue with these brodges anyway? Why the enable bits are not
> >> valid for them?
> 
> > Good question. I'd assumed because they are hotplugged and so no firmware
> > gets to set the bits properly ?
> 
>     The bits are strapped off the pins JP3/JP4.

Beats me then. Whatever - its easy enough to work around and avoid
exploding parisc and sparc so it definitely wants sorting

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Tue, 2011-04-19 at 22:28 +0100, Alan Cox wrote:
> Beats me then. Whatever - its easy enough to work around and avoid
> exploding parisc and sparc so it definitely wants sorting

OK, so are we all agreed on this (I'll split it up into the cosmetic
libata piece and the cmd64x fix later)?

James

---

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index f8380ce..b1b926c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 		return -ENOMEM;
 
 	if (!legacy_mode && pdev->irq) {
+		int i;
+
 		rc = devm_request_irq(dev, pdev->irq, irq_handler,
 				      IRQF_SHARED, drv_name, host);
 		if (rc)
 			goto out;
 
-		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
-		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
+		for (i = 0; i < 2; i++) {
+			if (ata_port_is_dummy(host->ports[i]))
+				continue;
+			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
+		}
 	} else if (legacy_mode) {
 		if (!ata_port_is_dummy(host->ports[0])) {
 			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index 905ff76..c39fd5a 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -41,6 +41,9 @@
 enum {
 	CFR 		= 0x50,
 		CFR_INTR_CH0  = 0x04,
+	CNTRL		= 0x51,
+		CNTRL_PRIMARY   = 0x04,
+		CNTRL_SECONDARY = 0x08,
 	CMDTIM 		= 0x52,
 	ARTTIM0 	= 0x53,
 	DRWTIM0 	= 0x54,
@@ -328,9 +331,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 			.port_ops = &cmd648_port_ops
 		}
 	};
-	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
-	u8 mrdmode;
+	const struct ata_port_info *ppi[] = { 
+		&cmd_info[id->driver_data],
+		&cmd_info[id->driver_data],
+		NULL
+	};
+	u8 mrdmode, reg;
 	int rc;
+	struct pci_dev *bridge = pdev->bus->self;
+	/* mobility split bridges don't report enabled ports correctly */
+	int port_ok = !(bridge && bridge->vendor ==
+			PCI_VENDOR_ID_MOBILITY_ELECTRONICS);
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -341,11 +352,15 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	if (pdev->device == PCI_DEVICE_ID_CMD_646) {
 		/* Does UDMA work ? */
-		if (pdev->revision > 4)
+		if (pdev->revision > 4) {
 			ppi[0] = &cmd_info[2];
+			ppi[1] = &cmd_info[2];
+		}
 		/* Early rev with other problems ? */
-		else if (pdev->revision == 1)
+		else if (pdev->revision == 1) {
 			ppi[0] = &cmd_info[3];
+			ppi[1] = &cmd_info[3];
+		}
 	}
 
 	pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
@@ -354,6 +369,21 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	mrdmode |= 0x02;	/* Memory read line enable */
 	pci_write_config_byte(pdev, MRDMODE, mrdmode);
 
+	/* check for enabled ports */
+	pci_read_config_byte(pdev, CNTRL, &reg);
+	if (port_ok)
+		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
+	/* 643 and 646 no UDMA, primary port always enabled */
+	if (port_ok && id->driver_data > 1 && !(reg & CNTRL_PRIMARY)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Primary port is disabled\n");
+		ppi[0] = &ata_dummy_port_info;
+		
+	}
+	if (port_ok && !(reg & CNTRL_SECONDARY)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n");
+		ppi[1] = &ata_dummy_port_info;
+	}
+
 	/* Force PIO 0 here.. */
 
 	/* PPC specific fixup copied from old driver */
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4e2c915..7a0ac45 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -608,6 +608,8 @@
 #define PCI_DEVICE_ID_MATROX_G550	0x2527
 #define PCI_DEVICE_ID_MATROX_VIA	0x4536
 
+#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS	0x14f2
+
 #define PCI_VENDOR_ID_CT		0x102c
 #define PCI_DEVICE_ID_CT_69000		0x00c0
 #define PCI_DEVICE_ID_CT_65545		0x00d8

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Alan Cox]

> +	struct pci_dev *bridge = pdev->bus->self;
> +	/* mobility split bridges don't report enabled ports correctly */
> +	int port_ok = !(bridge && bridge->vendor ==
> +			PCI_VENDOR_ID_MOBILITY_ELECTRONICS);
>  

The logic seems wrong even by inspection

port_ok will be 1 if it isn't a M/E bridge.

> +	/* check for enabled ports */
> +	pci_read_config_byte(pdev, CNTRL, &reg);
> +	if (port_ok)
> +		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");

If its *not* an ME bridge then print the warning ????


Otherwise looks right

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

On 20-04-2011 3:11, James Bottomley wrote:

>> Beats me then. Whatever - its easy enough to work around and avoid
>> exploding parisc and sparc so it definitely wants sorting

> OK, so are we all agreed on this (I'll split it up into the cosmetic
> libata piece and the cmd64x fix later)?

> James

> ---

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index f8380ce..b1b926c 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host,
>   		return -ENOMEM;
>
>   	if (!legacy_mode&&  pdev->irq) {
> +		int i;
> +
>   		rc = devm_request_irq(dev, pdev->irq, irq_handler,
>   				      IRQF_SHARED, drv_name, host);
>   		if (rc)
>   			goto out;
>
> -		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
> -		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
> +		for (i = 0; i < 2; i++) {
> +			if (ata_port_is_dummy(host->ports[i]))
> +				continue;
> +			ata_port_desc(host->ports[i], "irq %d", pdev->irq);

    Does this really makes any difference?

> diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
> index 905ff76..c39fd5a 100644
> --- a/drivers/ata/pata_cmd64x.c
> +++ b/drivers/ata/pata_cmd64x.c
> @@ -41,6 +41,9 @@
>   enum {
>   	CFR 		= 0x50,
>   		CFR_INTR_CH0  = 0x04,
> +	CNTRL		= 0x51,
> +		CNTRL_PRIMARY   = 0x04,
> +		CNTRL_SECONDARY = 0x08,

    Probably better to call them CNTRL_CH0 and CNTRL_CH1 to keep the naming in 
line with already existing one...

> @@ -328,9 +331,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   			.port_ops =&cmd648_port_ops
>   		}
>   	};
> -	const struct ata_port_info *ppi[] = {&cmd_info[id->driver_data], NULL };
> -	u8 mrdmode;
> +	const struct ata_port_info *ppi[] = {
> +		&cmd_info[id->driver_data],
> +		&cmd_info[id->driver_data],
> +		NULL
> +	};
> +	u8 mrdmode, reg;
>   	int rc;
> +	struct pci_dev *bridge = pdev->bus->self;
> +	/* mobility split bridges don't report enabled ports correctly */
> +	int port_ok = !(bridge && bridge->vendor ==
> +			PCI_VENDOR_ID_MOBILITY_ELECTRONICS);
>
>   	rc = pcim_enable_device(pdev);
>   	if (rc)
> @@ -354,6 +369,21 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mrdmode |= 0x02;	/* Memory read line enable */
>   	pci_write_config_byte(pdev, MRDMODE, mrdmode);
>
> +	/* check for enabled ports */
> +	pci_read_config_byte(pdev, CNTRL,&reg);
> +	if (port_ok)

    You mean !port_ok.

> +		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
> +	/* 643 and 646 no UDMA, primary port always enabled */
> +	if (port_ok && id->driver_data > 1 &&  !(reg & CNTRL_PRIMARY)) {

    PCI0646U and later revisions on PCI0646 do have the primary port enable 
bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in 
cmd64x_init_one()...

> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 4e2c915..7a0ac45 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -608,6 +608,8 @@
>   #define PCI_DEVICE_ID_MATROX_G550	0x2527
>   #define PCI_DEVICE_ID_MATROX_VIA	0x4536
>
> +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS	0x14f2
> +

    The current trend seems to be to only define vendor/device IDs where they 
are used and not in pci_ids.h...

MBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Wed, 2011-04-20 at 14:04 +0400, Sergei Shtylyov wrote:

> > +		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
> > +	/* 643 and 646 no UDMA, primary port always enabled */
> > +	if (port_ok && id->driver_data > 1 &&  !(reg & CNTRL_PRIMARY)) {
> 
>     PCI0646U and later revisions on PCI0646 do have the primary port enable 
> bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in 
> cmd64x_init_one()...

Where?  All I see in drivers/ide/cmd64x.c is that it only ignores the
primary for the id->driver_data == 0 case, which is what I originally
coded.

James

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Wed, 2011-04-20 at 09:28 -0500, James Bottomley wrote:
> On Wed, 2011-04-20 at 14:04 +0400, Sergei Shtylyov wrote:
> 
> > > +		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
> > > +	/* 643 and 646 no UDMA, primary port always enabled */
> > > +	if (port_ok && id->driver_data > 1 &&  !(reg & CNTRL_PRIMARY)) {
> > 
> >     PCI0646U and later revisions on PCI0646 do have the primary port enable 
> > bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in 
> > cmd64x_init_one()...
> 
> Where?  All I see in drivers/ide/cmd64x.c is that it only ignores the
> primary for the id->driver_data == 0 case, which is what I originally
> coded.

OK, found it ... it's the pdev->revision < 3 check.  

James

---

>From 71be695c796eeaed7b45b3756a101f87b77827c2 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 18 Apr 2011 13:52:36 -0700
Subject: [PATCH] pata_cm64x: latest fix for boot crash


diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index f8380ce..b1b926c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 		return -ENOMEM;
 
 	if (!legacy_mode && pdev->irq) {
+		int i;
+
 		rc = devm_request_irq(dev, pdev->irq, irq_handler,
 				      IRQF_SHARED, drv_name, host);
 		if (rc)
 			goto out;
 
-		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
-		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
+		for (i = 0; i < 2; i++) {
+			if (ata_port_is_dummy(host->ports[i]))
+				continue;
+			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
+		}
 	} else if (legacy_mode) {
 		if (!ata_port_is_dummy(host->ports[0])) {
 			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index 905ff76..7bafc16 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -41,6 +41,9 @@
 enum {
 	CFR 		= 0x50,
 		CFR_INTR_CH0  = 0x04,
+	CNTRL		= 0x51,
+		CNTRL_CH0     = 0x04,
+		CNTRL_CH1     = 0x08,
 	CMDTIM 		= 0x52,
 	ARTTIM0 	= 0x53,
 	DRWTIM0 	= 0x54,
@@ -328,9 +331,19 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 			.port_ops = &cmd648_port_ops
 		}
 	};
-	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
-	u8 mrdmode;
+	const struct ata_port_info *ppi[] = { 
+		&cmd_info[id->driver_data],
+		&cmd_info[id->driver_data],
+		NULL
+	};
+	u8 mrdmode, reg;
 	int rc;
+	struct pci_dev *bridge = pdev->bus->self;
+	/* mobility split bridges don't report enabled ports correctly */
+	int port_ok = !(bridge && bridge->vendor ==
+			PCI_VENDOR_ID_MOBILITY_ELECTRONICS);
+	/* all (with exceptions below) apart from 643 have CNTRL_CH0 bit */
+	int cntrl_ch0_ok = (id->driver_data != 0);
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -341,11 +354,18 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	if (pdev->device == PCI_DEVICE_ID_CMD_646) {
 		/* Does UDMA work ? */
-		if (pdev->revision > 4)
+		if (pdev->revision > 4) {
 			ppi[0] = &cmd_info[2];
+			ppi[1] = &cmd_info[2];
+		}
 		/* Early rev with other problems ? */
-		else if (pdev->revision == 1)
+		else if (pdev->revision == 1) {
 			ppi[0] = &cmd_info[3];
+			ppi[1] = &cmd_info[3];
+		}
+		/* revs 1,2 have no CNTRL_CH0 */
+		if (pdev->revision < 3)
+			cntrl_ch0_ok = 0;
 	}
 
 	pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
@@ -354,6 +374,20 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	mrdmode |= 0x02;	/* Memory read line enable */
 	pci_write_config_byte(pdev, MRDMODE, mrdmode);
 
+	/* check for enabled ports */
+	pci_read_config_byte(pdev, CNTRL, &reg);
+	if (!port_ok)
+		dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
+	if (port_ok && cntrl_ch0_ok && !(reg & CNTRL_CH0)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Primary port is disabled\n");
+		ppi[0] = &ata_dummy_port_info;
+		
+	}
+	if (port_ok && !(reg & CNTRL_CH1)) {
+		dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n");
+		ppi[1] = &ata_dummy_port_info;
+	}
+
 	/* Force PIO 0 here.. */
 
 	/* PPC specific fixup copied from old driver */
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4e2c915..7a0ac45 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -608,6 +608,8 @@
 #define PCI_DEVICE_ID_MATROX_G550	0x2527
 #define PCI_DEVICE_ID_MATROX_VIA	0x4536
 
+#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS	0x14f2
+
 #define PCI_VENDOR_ID_CT		0x102c
 #define PCI_DEVICE_ID_CT_69000		0x00c0
 #define PCI_DEVICE_ID_CT_65545		0x00d8

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

On 20-04-2011 18:28, James Bottomley wrote:

>>> +		dev_printk(KERN_NOTICE,&pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n");
>>> +	/* 643 and 646 no UDMA, primary port always enabled */
>>> +	if (port_ok&&  id->driver_data>  1&&   !(reg&  CNTRL_PRIMARY)) {

    This should probably be:

	if (port_ok && !(id->driver_data == 0 || id->driver_data == 1 &&
	    pdev->revision < 3) && !(reg & CNTRL_PRIMARY)) {

>>      PCI0646U and later revisions on PCI0646 do have the primary port enable
>> bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in
>> cmd64x_init_one()...

> Where?  All I see in drivers/ide/cmd64x.c is that it only ignores the
> primary for the id->driver_data == 0 case, which is what I originally
> coded.

    Hm, are we looking at the same driver?

static const struct ide_port_info cmd64x_chipsets[] __devinitdata = {
	{	/* 0: CMD643 */
		.name		= DRV_NAME,
		.init_chipset	= init_chipset_cmd64x,
		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
		.port_ops	= &cmd64x_port_ops,
		.host_flags	= IDE_HFLAG_CLEAR_SIMPLEX |
				  IDE_HFLAG_ABUSE_PREFETCH |
				  IDE_HFLAG_SERIALIZE,
		.pio_mask	= ATA_PIO5,
		.mwdma_mask	= ATA_MWDMA2,
		.udma_mask	= 0x00, /* no udma */
	},
	{	/* 1: CMD646 */
		.name		= DRV_NAME,
		.init_chipset	= init_chipset_cmd64x,
		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	= &cmd648_port_ops,
		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH |
				  IDE_HFLAG_SERIALIZE,
		.pio_mask	= ATA_PIO5,
		.mwdma_mask	= ATA_MWDMA2,
		.udma_mask	= ATA_UDMA2,
	},
	{	/* 2: CMD648 */
		.name		= DRV_NAME,
		.init_chipset	= init_chipset_cmd64x,
		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	= &cmd648_port_ops,
		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH,
		.pio_mask	= ATA_PIO5,
		.mwdma_mask	= ATA_MWDMA2,
		.udma_mask	= ATA_UDMA4,
	},
	{	/* 3: CMD649 */
		.name		= DRV_NAME,
		.init_chipset	= init_chipset_cmd64x,
		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	= &cmd648_port_ops,
		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH,
		.pio_mask	= ATA_PIO5,
		.mwdma_mask	= ATA_MWDMA2,
		.udma_mask	= ATA_UDMA5,
	}
};

static int __devinit cmd64x_init_one(struct pci_dev *dev, const struct 
pci_device_id *id)
{
	struct ide_port_info d;
	u8 idx = id->driver_data;

	d = cmd64x_chipsets[idx];

	if (idx == 1) {
		/*
		 * UltraDMA only supported on PCI646U and PCI646U2, which
		 * correspond to revisions 0x03, 0x05 and 0x07 respectively.
		 * Actually, although the CMD tech support people won't
		 * tell me the details, the 0x03 revision cannot support
		 * UDMA correctly without hardware modifications, and even
		 * then it only works with Quantum disks due to some
		 * hold time assumptions in the 646U part which are fixed
		 * in the 646U2.
		 *
		 * So we only do UltraDMA on revision 0x05 and 0x07 chipsets.
		 */
		if (dev->revision < 5) {
			d.udma_mask = 0x00;
			/*
			 * The original PCI0646 didn't have the primary
			 * channel enable bit, it appeared starting with
			 * PCI0646U (i.e. revision ID 3).
			 */
			if (dev->revision < 3) {
				d.enablebits[0].reg = 0;
				d.port_ops = &cmd64x_port_ops;
				if (dev->revision == 1)
					d.dma_ops = &cmd646_rev1_dma_ops;
			}
		}
	}

	return ide_pci_init_one(dev, &d, NULL);
}

static const struct pci_device_id cmd64x_pci_tbl[] = {
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_643), 0 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_646), 1 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_648), 2 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_649), 3 },
	{ 0, },
};
MODULE_DEVICE_TABLE(pci, cmd64x_pci_tbl);

    "ïdx == 1" corresponds to PCI0646. See this "dev->revision < 3" check 
(this is true for the original PCI0646), where it then zeroes the 'reg' field 
of 'enablebits' to disable its checking?

> James

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

On 20-04-2011 18:28, James Bottomley wrote:

>>> +		dev_printk(KERN_NOTICE,&pdev->dev, "Mobility Bridge detected, ig=
noring CNTRL port enable/disable\n");
>>> +	/* 643 and 646 no UDMA, primary port always enabled */
>>> +	if (port_ok&&  id->driver_data>  1&&   !(reg&  CNTRL_PRIMARY)) {

    This should probably be:

	if (port_ok && !(id->driver_data =3D=3D 0 || id->driver_data =3D=3D 1 =
&&
	    pdev->revision < 3) && !(reg & CNTRL_PRIMARY)) {

>>      PCI0646U and later revisions on PCI0646 do have the primary por=
t enable
>> bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64=
x does in
>> cmd64x_init_one()...

> Where?  All I see in drivers/ide/cmd64x.c is that it only ignores the
> primary for the id->driver_data =3D=3D 0 case, which is what I origin=
ally
> coded.

    Hm, are we looking at the same driver?

static const struct ide_port_info cmd64x_chipsets[] __devinitdata =3D {
	{	/* 0: CMD643 */
		.name		=3D DRV_NAME,
		.init_chipset	=3D init_chipset_cmd64x,
		.enablebits	=3D {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
		.port_ops	=3D &cmd64x_port_ops,
		.host_flags	=3D IDE_HFLAG_CLEAR_SIMPLEX |
				  IDE_HFLAG_ABUSE_PREFETCH |
				  IDE_HFLAG_SERIALIZE,
		.pio_mask	=3D ATA_PIO5,
		.mwdma_mask	=3D ATA_MWDMA2,
		.udma_mask	=3D 0x00, /* no udma */
	},
	{	/* 1: CMD646 */
		.name		=3D DRV_NAME,
		.init_chipset	=3D init_chipset_cmd64x,
		.enablebits	=3D {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	=3D &cmd648_port_ops,
		.host_flags	=3D IDE_HFLAG_ABUSE_PREFETCH |
				  IDE_HFLAG_SERIALIZE,
		.pio_mask	=3D ATA_PIO5,
		.mwdma_mask	=3D ATA_MWDMA2,
		.udma_mask	=3D ATA_UDMA2,
	},
	{	/* 2: CMD648 */
		.name		=3D DRV_NAME,
		.init_chipset	=3D init_chipset_cmd64x,
		.enablebits	=3D {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	=3D &cmd648_port_ops,
		.host_flags	=3D IDE_HFLAG_ABUSE_PREFETCH,
		.pio_mask	=3D ATA_PIO5,
		.mwdma_mask	=3D ATA_MWDMA2,
		.udma_mask	=3D ATA_UDMA4,
	},
	{	/* 3: CMD649 */
		.name		=3D DRV_NAME,
		.init_chipset	=3D init_chipset_cmd64x,
		.enablebits	=3D {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
		.port_ops	=3D &cmd648_port_ops,
		.host_flags	=3D IDE_HFLAG_ABUSE_PREFETCH,
		.pio_mask	=3D ATA_PIO5,
		.mwdma_mask	=3D ATA_MWDMA2,
		.udma_mask	=3D ATA_UDMA5,
	}
};

static int __devinit cmd64x_init_one(struct pci_dev *dev, const struct=20
pci_device_id *id)
{
	struct ide_port_info d;
	u8 idx =3D id->driver_data;

	d =3D cmd64x_chipsets[idx];

	if (idx =3D=3D 1) {
		/*
		 * UltraDMA only supported on PCI646U and PCI646U2, which
		 * correspond to revisions 0x03, 0x05 and 0x07 respectively.
		 * Actually, although the CMD tech support people won't
		 * tell me the details, the 0x03 revision cannot support
		 * UDMA correctly without hardware modifications, and even
		 * then it only works with Quantum disks due to some
		 * hold time assumptions in the 646U part which are fixed
		 * in the 646U2.
		 *
		 * So we only do UltraDMA on revision 0x05 and 0x07 chipsets.
		 */
		if (dev->revision < 5) {
			d.udma_mask =3D 0x00;
			/*
			 * The original PCI0646 didn't have the primary
			 * channel enable bit, it appeared starting with
			 * PCI0646U (i.e. revision ID 3).
			 */
			if (dev->revision < 3) {
				d.enablebits[0].reg =3D 0;
				d.port_ops =3D &cmd64x_port_ops;
				if (dev->revision =3D=3D 1)
					d.dma_ops =3D &cmd646_rev1_dma_ops;
			}
		}
	}

	return ide_pci_init_one(dev, &d, NULL);
}

static const struct pci_device_id cmd64x_pci_tbl[] =3D {
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_643), 0 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_646), 1 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_648), 2 },
	{ PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_649), 3 },
	{ 0, },
};
MODULE_DEVICE_TABLE(pci, cmd64x_pci_tbl);

    "=C3=AFdx =3D=3D 1" corresponds to PCI0646. See this "dev->revision=
 < 3" check=20
(this is true for the original PCI0646), where it then zeroes the 'reg'=
 field=20
of 'enablebits' to disable its checking?

> James

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Matthew Wilcox]

On Wed, Apr 20, 2011 at 02:04:10PM +0400, Sergei Shtylyov wrote:
>> +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS	0x14f2
>> +
>
>    The current trend seems to be to only define vendor/device IDs where 
> they are used and not in pci_ids.h...

Device IDs, yes.  Vendor IDs should always go to the pci_ids.h file, since
they're likely to be used in multiple places.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Jeff Garzik]

On 04/20/2011 10:56 AM, Matthew Wilcox wrote:
> On Wed, Apr 20, 2011 at 02:04:10PM +0400, Sergei Shtylyov wrote:
>>> +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS	0x14f2
>>> +
>>
>>     The current trend seems to be to only define vendor/device IDs where
>> they are used and not in pci_ids.h...
>
> Device IDs, yes.  Vendor IDs should always go to the pci_ids.h file, since
> they're likely to be used in multiple places.

Correct; that's the current libata policy.

	Jeff

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>> On Sun, 2011-04-24 at 14:28 -0500, James Bottomley wrote:
>>> currently libata-sff is completely ignoring the enabled/disabled status
>>> of the interfaces.  This is a real problem on parisc because if you
>>> touch a non responding memory area (i.e. a disabled interface) you crash
>>> the box.

>>> Fix by checking the CNTRL bits to see if the port is enabled before
>>> trying to poke it.

>> Ping on this.

>> Since 1/2 is an essential fix to prevent a boot panic on parisc, I can
>> just take them through the parisc tree.

> It's in libata-dev.git#upstream and #NEXT (linux-next) at present.

    I'm wondering about the other drivers that check the channel enable bits in 
their prereset() methods. James has shown that such code would still crash on 
such platforms as PARISC (I suspect many more platfroms which don't silently 
ignore the target aborts in the PCI space like x86 does). I think all such 
drivers should be converted to the early port disable detection scheme used in 
the pata_cmd64x driver (an some others)...

>     Jeff

WBR, Sergei

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[Jeff Garzik]

On 05/13/2011 01:01 PM, James Bottomley wrote:
> On Sun, 2011-04-24 at 14:28 -0500, James Bottomley wrote:
>> currently libata-sff is completely ignoring the enabled/disabled status
>> of the interfaces.  This is a real problem on parisc because if you
>> touch a non responding memory area (i.e. a disabled interface) you crash
>> the box.
>>
>> Fix by checking the CNTRL bits to see if the port is enabled before
>> trying to poke it.
>
> Ping on this.
>
> Since 1/2 is an essential fix to prevent a boot panic on parisc, I can
> just take them through the parisc tree.

It's in libata-dev.git#upstream and #NEXT (linux-next) at present.

	Jeff

Re: [PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

On Sun, 2011-04-24 at 14:28 -0500, James Bottomley wrote:
> currently libata-sff is completely ignoring the enabled/disabled status
> of the interfaces.  This is a real problem on parisc because if you
> touch a non responding memory area (i.e. a disabled interface) you crash
> the box.
> 
> Fix by checking the CNTRL bits to see if the port is enabled before
> trying to poke it.

Ping on this.

Since 1/2 is an essential fix to prevent a boot panic on parisc, I can
just take them through the parisc tree.

James


> James
> 
> ---
> 
> James Bottomley (2):
>   pata_cm64x: fix boot crash on parisc
>   libata-sff: prevent irq descriptions for dummy ports
> 
>  drivers/ata/libata-sff.c  |    9 +++++++--
>  drivers/ata/pata_cmd64x.c |   42 ++++++++++++++++++++++++++++++++++++++----
>  include/linux/pci_ids.h   |    2 ++
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 

[PATCH 0/2] fix libata-sff and pata_cmd64x to not crash on boot on parisc

[James Bottomley]

currently libata-sff is completely ignoring the enabled/disabled status
of the interfaces.  This is a real problem on parisc because if you
touch a non responding memory area (i.e. a disabled interface) you crash
the box.

Fix by checking the CNTRL bits to see if the port is enabled before
trying to poke it.

James

---

James Bottomley (2):
  pata_cm64x: fix boot crash on parisc
  libata-sff: prevent irq descriptions for dummy ports

 drivers/ata/libata-sff.c  |    9 +++++++--
 drivers/ata/pata_cmd64x.c |   42 ++++++++++++++++++++++++++++++++++++++----
 include/linux/pci_ids.h   |    2 ++
 3 files changed, 47 insertions(+), 6 deletions(-)

-- 
1.7.4.1