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

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

Comments

James Bottomley - April 18, 2011, 6:45 p.m.
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(-)
Sergei Shtylyov - April 19, 2011, 8:48 p.m.
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

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