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

login
register
mail settings
Submitter James Bottomley
Date April 18, 2011, 8:50 p.m.
Message ID <1303159802.7167.30.camel@mulgrave.site>
Download mbox | patch
Permalink /patch/91841/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

James Bottomley - April 18, 2011, 8:50 p.m.
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

---



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 18, 2011, 9:20 p.m.
>	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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - April 19, 2011, 1:54 p.m.
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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 19, 2011, 2:36 p.m.
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - April 19, 2011, 3:02 p.m.
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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 19, 2011, 3:58 p.m.
> > 	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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - April 19, 2011, 8:53 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - April 19, 2011, 8:59 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 19, 2011, 9:19 p.m.
> > 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 ?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - April 19, 2011, 9:22 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 19, 2011, 9:28 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */