[PATCH 1/3] Add disk hotswap support to libata RESEND #5

[PATCH 1/3] Add disk hotswap support to libata RESEND #5

[Lukasz Kosewski]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

Hey Jeff, all,

Patch 1/3 for libata hotswapping, properly masking out hotplug
interrupts on Promise SATAII150 line of controllers.

More comments available in patch and header, please review and apply
if you like it!

Luke Kosewski

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 01-promise_sataII150_support-2.6.14-rc2-git5.diff --]
[-- Type: text/x-patch; name="01-promise_sataII150_support-2.6.14-rc2-git5.diff", Size: 7778 bytes --]

26.09.05    Luke Kosewski   <lkosewsk@nit.ca>

	* A patch to the sata_promise driver in libata for it to correctly mask
	  out hotplug interrupts on SATAII150 Tx4/Tx2 Plus controllers.
	* This is resend #4.  Now no longer memory leaking struct
	  pdc_host_privs all over the place on controller hot-unplugs.

diff -rpuN linux-2.6.14-rc1/drivers/scsi/sata_promise.c linux-2.6.14-rc1-new/drivers/scsi/sata_promise.c
--- linux-2.6.14-rc1/drivers/scsi/sata_promise.c	2005-09-12 23:12:09.000000000 -0400
+++ linux-2.6.14-rc1-new/drivers/scsi/sata_promise.c	2005-09-14 14:11:49.000000000 -0400
@@ -57,6 +57,7 @@ enum {
 	PDC_GLOBAL_CTL		= 0x48, /* Global control/status (per port) */
 	PDC_CTLSTAT		= 0x60,	/* IDE control and status (per port) */
 	PDC_SATA_PLUG_CSR	= 0x6C, /* SATA Plug control/status reg */
+	PDC2_SATA_PLUG_CSR	= 0X60, /* SATAII Plug control/status reg */
 	PDC_SLEW_CTL		= 0x470, /* slew rate control reg */
 
 	PDC_ERR_MASK		= (1<<19) | (1<<20) | (1<<21) | (1<<22) |
@@ -65,8 +66,10 @@ enum {
 	board_2037x		= 0,	/* FastTrak S150 TX2plus */
 	board_20319		= 1,	/* FastTrak S150 TX4 */
 	board_20619		= 2,	/* FastTrak TX4000 */
+	board_2057x		= 3,	/* SATAII150 Tx2plus */
+	board_40518		= 4,	/* SATAII150 Tx4 */
 
-	PDC_HAS_PATA		= (1 << 1), /* PDC20375 has PATA */
+	PDC_HAS_PATA		= (1 << 1), /* PDC20375/20575 has PATA */
 
 	PDC_RESET		= (1 << 11), /* HDMA reset */
 };
@@ -77,6 +80,10 @@ struct pdc_port_priv {
 	dma_addr_t		pkt_dma;
 };
 
+struct pdc_host_priv {
+	int			hotplug_offset;
+};
+
 static u32 pdc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void pdc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int pdc_ata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
@@ -98,6 +105,7 @@
 static void pdc_exec_command_mmio(struct ata_port *ap, struct ata_taskfile *tf);
 static void pdc_irq_clear(struct ata_port *ap);
 static int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
+static void pdc_host_stop(struct ata_host_set *host_set);
 
 
 static Scsi_Host_Template pdc_ata_sht = {
@@ -141,7 +148,7 @@ static struct ata_port_operations pdc_sa
	.scr_write		= pdc_sata_scr_write,
	.port_start		= pdc_port_start,
	.port_stop		= pdc_port_stop,
-	.host_stop		= ata_pci_host_stop,
+	.host_stop		= pdc_host_stop,
 };
 
 static struct ata_port_operations pdc_pata_ops = {
@@ -162,7 +169,7 @@ static struct ata_port_operations pdc_pa
 
	.port_start		= pdc_port_start,
	.port_stop		= pdc_port_stop,
-	.host_stop		= ata_pci_host_stop,
+	.host_stop		= pdc_host_stop,
 };
 
 static struct ata_port_info pdc_port_info[] = {
@@ -190,6 +198,28 @@ static struct ata_port_info pdc_port_inf
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &pdc_pata_ops,
 	},
+
+	/* board_2057x */
+	{
+		.sht		= &pdc_ata_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.mwdma_mask	= 0x07, /* mwdma0-2 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &pdc_sata_ops,
+	},
+
+	/* board_40518 */
+	{
+		.sht		= &pdc_ata_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.mwdma_mask	= 0x07, /* mwdma0-2 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &pdc_sata_ops,
+	},
 };
 
 static struct pci_device_id pdc_ata_pci_tbl[] = {
@@ -204,9 +234,9 @@ static struct pci_device_id pdc_ata_pci_
 	{ PCI_VENDOR_ID_PROMISE, 0x3376, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_2037x },
 	{ PCI_VENDOR_ID_PROMISE, 0x3574, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-	  board_2037x },
+	  board_2057x },
 	{ PCI_VENDOR_ID_PROMISE, 0x3d75, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-	  board_2037x },
+	  board_2057x },
 
 	{ PCI_VENDOR_ID_PROMISE, 0x3318, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_20319 },
@@ -217,7 +247,7 @@ static struct pci_device_id pdc_ata_pci_
 	{ PCI_VENDOR_ID_PROMISE, 0x3d17, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_20319 },
 	{ PCI_VENDOR_ID_PROMISE, 0x3d18, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-	  board_20319 },
+	  board_40518 },
 
 	{ PCI_VENDOR_ID_PROMISE, 0x6629, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_20619 },
@@ -311,6 +331,16 @@ static void pdc_port_stop(struct ata_por
 }
 
 
+static void pdc_host_stop(struct ata_host_set *host_set)
+{
+	struct pdc_host_priv *hp = host_set->private_data;
+
+	ata_pci_host_stop(host_set);
+
+	kfree(hp);  /* Clean up our mess */
+}
+
+
 static void pdc_reset_port(struct ata_port *ap)
 {
	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_CTLSTAT;
@@ -478,14 +518,15 @@ static irqreturn_t pdc_interrupt (int ir
 		VPRINTK("QUICK EXIT 2\n");
 		return IRQ_NONE;
 	}
+
+	spin_lock(&host_set->lock);
+
 	mask &= 0xffff;		/* only 16 tags possible */
 	if (!mask) {
 		VPRINTK("QUICK EXIT 3\n");
-		return IRQ_NONE;
+		goto done_irq;
 	}
 
-	spin_lock(&host_set->lock);
-
 	writel(mask, mmio_base + PDC_INT_SEQMASK);
 
 	for (i = 0; i < host_set->n_ports; i++) {
@@ -502,10 +543,10 @@ static irqreturn_t pdc_interrupt (int ir
 		}
 	}
 
-        spin_unlock(&host_set->lock);
-
 	VPRINTK("EXIT\n");
 
+done_irq:
+	spin_unlock(&host_set->lock);
 	return IRQ_RETVAL(handled);
 }
 
@@ -583,6 +624,8 @@ static void pdc_ata_setup_port(struct at
 static void pdc_host_init(unsigned int chip_id, struct ata_probe_ent *pe)
 {
 	void __iomem *mmio = pe->mmio_base;
+	struct pdc_host_priv *hp = pe->private_data;
+	int hotplug_offset = hp->hotplug_offset;
 	u32 tmp;
 
 	/*
@@ -597,12 +640,12 @@ static void pdc_host_init(unsigned int c
 	writel(tmp, mmio + PDC_FLASH_CTL);
 
 	/* clear plug/unplug flags for all ports */
-	tmp = readl(mmio + PDC_SATA_PLUG_CSR);
-	writel(tmp | 0xff, mmio + PDC_SATA_PLUG_CSR);
+	tmp = readl(mmio + hotplug_offset);
+	writel(tmp | 0xff, mmio + hotplug_offset);
 
 	/* mask plug/unplug ints */
-	tmp = readl(mmio + PDC_SATA_PLUG_CSR);
-	writel(tmp | 0xff0000, mmio + PDC_SATA_PLUG_CSR);
+	tmp = readl(mmio + hotplug_offset);
+	writel(tmp | 0xff0000, mmio + hotplug_offset);
 
 	/* reduce TBG clock to 133 Mhz. */
 	tmp = readl(mmio + PDC_TBG_MODE);
@@ -624,6 +667,7 @@ static int pdc_ata_init_one (struct pci_
 {
 	static int printed_version;
 	struct ata_probe_ent *probe_ent = NULL;
+	struct pdc_host_priv *hp;
 	unsigned long base;
 	void __iomem *mmio_base;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
@@ -671,6 +715,17 @@ static int pdc_ata_init_one (struct pci_
 	}
 	base = (unsigned long) mmio_base;
 
+	hp = kmalloc(sizeof(*hp), GFP_KERNEL);
+	if (hp == NULL) {
+		rc = -ENOMEM;
+		goto err_out_free_ent;
+	}
+	memset(hp, 0, sizeof(*hp));
+
+	/* Set default hotplug offset */
+	hp->hotplug_offset = PDC_SATA_PLUG_CSR;
+	probe_ent->private_data = hp;
+
 	probe_ent->sht		= pdc_port_info[board_idx].sht;
 	probe_ent->host_flags	= pdc_port_info[board_idx].host_flags;
 	probe_ent->pio_mask	= pdc_port_info[board_idx].pio_mask;
@@ -690,6 +745,9 @@ static int pdc_ata_init_one (struct pci_
 
 	/* notice 4-port boards */
 	switch (board_idx) {
+	case board_40518:
+		/* Override hotplug offset for SATAII150 */
+		hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
 	case board_20319:
        		probe_ent->n_ports = 4;
 
@@ -699,6 +757,9 @@ static int pdc_ata_init_one (struct pci_
 		probe_ent->port[2].scr_addr = base + 0x600;
 		probe_ent->port[3].scr_addr = base + 0x700;
 		break;
+	case board_2057x:
+		/* Override hotplug offset for SATAII150 */
+		hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
 	case board_2037x:
        		probe_ent->n_ports = 2;
 		break;
@@ -724,7 +785,7 @@ static int pdc_ata_init_one (struct pci_
	/* initialize adapter */
	pdc_host_init(board_idx, probe_ent);
 
-	/* FIXME: check ata_device_add return value */
+	/* FIXME: check ata_device_add return value.  If 0, kfree(hp) */
	ata_device_add(probe_ent);
	kfree(probe_ent);
 

Re: [PATCH 1/3] Add disk hotswap support to libata RESEND #5

[Jeff Garzik]

Lukasz Kosewski wrote:
> @@ -57,6 +57,7 @@ enum {
>  	PDC_GLOBAL_CTL		= 0x48, /* Global control/status (per port) */
>  	PDC_CTLSTAT		= 0x60,	/* IDE control and status (per port) */
>  	PDC_SATA_PLUG_CSR	= 0x6C, /* SATA Plug control/status reg */
> +	PDC2_SATA_PLUG_CSR	= 0X60, /* SATAII Plug control/status reg */

Did you actually compile and test this?  :)


> @@ -690,6 +745,9 @@ static int pdc_ata_init_one (struct pci_
>  
>  	/* notice 4-port boards */
>  	switch (board_idx) {
> +	case board_40518:
> +		/* Override hotplug offset for SATAII150 */
> +		hp->hotplug_offset = PDC2_SATA_PLUG_CSR;

add a comment /* fall through */ here


>  	case board_20319:
>         		probe_ent->n_ports = 4;
>  
> @@ -699,6 +757,9 @@ static int pdc_ata_init_one (struct pci_
>  		probe_ent->port[2].scr_addr = base + 0x600;
>  		probe_ent->port[3].scr_addr = base + 0x700;
>  		break;
> +	case board_2057x:
> +		/* Override hotplug offset for SATAII150 */
> +		hp->hotplug_offset = PDC2_SATA_PLUG_CSR;

ditto


>  	case board_2037x:
>         		probe_ent->n_ports = 2;
>  		break;
> @@ -724,7 +785,7 @@ static int pdc_ata_init_one (struct pci_
> 	/* initialize adapter */
> 	pdc_host_init(board_idx, probe_ent);
>  
> -	/* FIXME: check ata_device_add return value */
> +	/* FIXME: check ata_device_add return value.  If 0, kfree(hp) */
> 	ata_device_add(probe_ent);

Just leave the comment as is.  You made it worse:

* if ata_device_add() returns zero, then everything is OK.

* if ata_device_add() returns non-zero, then an error occured. 
kfree(hp) is but one of several things that need to be cleaned up on 
failure.


Finally, please fix the format of your subject line per
	http://linux.yyz.us/patch-format.html

Most notably, each Subject should be unique for each patch.  e.g.

[PATCH 1/3] sata_promise: fix hotplug register offset
[PATCH 2/3] libata: add device hotplug infrastructure
[PATCH 3/3] sata_promise: add device hotplug support

	Jeff

Re: [PATCH 1/3] Add disk hotswap support to libata RESEND #5

[Lukasz Kosewski]

On 9/28/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> > +     PDC2_SATA_PLUG_CSR      = 0X60, /* SATAII Plug control/status reg */
>
> Did you actually compile and test this?  :)

Yes.  And it works, which is why the compiling and testing doesn't
catch it :P  I'll change it.

> > +     case board_40518:
> > +             /* Override hotplug offset for SATAII150 */
> > +             hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
>
> add a comment /* fall through */ here

OK.

> > -     /* FIXME: check ata_device_add return value */
> > +     /* FIXME: check ata_device_add return value.  If 0, kfree(hp) */
> >       ata_device_add(probe_ent);
>
> Just leave the comment as is.  You made it worse:
>
> * if ata_device_add() returns zero, then everything is OK.
>
> * if ata_device_add() returns non-zero, then an error occured.
> kfree(hp) is but one of several things that need to be cleaned up on
> failure.

No.  ata_device_add returns nonzero on success; so say the docs. 
Since the return value is not checked here, and whether on success or
failure all of the data structures allocated in that method stick
around, I assumed that something was in the works for this.  I'll
change this to kfree(hp) on returning 0.  Please advise if I should do
something else.

> Finally, please fix the format of your subject line per
>         http://linux.yyz.us/patch-format.html
>
> Most notably, each Subject should be unique for each patch.  e.g.
>
> [PATCH 1/3] sata_promise: fix hotplug register offset
> [PATCH 2/3] libata: add device hotplug infrastructure
> [PATCH 3/3] sata_promise: add device hotplug support

OK.

Luke

Re: [PATCH 1/3] Add disk hotswap support to libata RESEND #5

[Jeff Garzik]

Lukasz Kosewski wrote:
> No.  ata_device_add returns nonzero on success; so say the docs. 

Whoops, you're right.  I forget it was special.


> Since the return value is not checked here, and whether on success or
> failure all of the data structures allocated in that method stick
> around, I assumed that something was in the works for this.  I'll
> change this to kfree(hp) on returning 0.  Please advise if I should do
> something else.

It still needs to clean up after pdc_host_init()...

	Jeff