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 19, 2011, 11:11 p.m.
Message ID <1303254709.11237.34.camel@mulgrave.site>
Download mbox | patch
Permalink /patch/92067/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

James Bottomley - April 19, 2011, 11:11 p.m.
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

---



--
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 20, 2011, 9:35 a.m.
> +	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
--
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 20, 2011, 10:04 a.m.
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
--
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 20, 2011, 2:28 p.m.
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


--
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 20, 2011, 2:54 p.m.
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-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Wilcox - April 20, 2011, 2:56 p.m.
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.
Jeff Garzik - April 21, 2011, 2:24 p.m.
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



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