Re: [PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

Re: [PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

[daniel]

>Wolstenholme, Daniel E wrote:
>> Ok, hopefully I can get the attachment right here.
>> 
>> Here's a resubmission of my MSI patch for the sata_vsc driver.
>> 
>> I've added a new vsc_sata_host_priv data structure (basically copied
>> from the sata_mv driver) which has an hp_flags bitfield with a bit to
>> indicate whether MSI is being used. This is used in the new
>> vsc_sata_host_stop() routine to call
>> pci_disable_msi() if MSI is on, pci_intx() if not.
>> 
>> Signed-off-by: Daniel Wolstenholme <daniel.e.wolstenholme@intel.com>
>
>ACK the patch technical content.
>
>Unfortunately, the patch was corrupted, and tabs were converted to 
>spaces, which makes it impossible to apply the patch.apply the patch.

Ok, this one should get it right.  This makes me miss the days of elm and no 
firewalls.
Dan


---------------

diff -uprN a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c	2006-09-19 20:42:06.000000000 -0700
+++ b/drivers/scsi/sata_vsc.c	2006-10-16 12:11:43.000000000 -0700
@@ -94,8 +94,14 @@ enum {
 			      VSC_SATA_INT_ERROR_P    | VSC_SATA_INT_ERROR_R | \
 			      VSC_SATA_INT_ERROR_E    | VSC_SATA_INT_ERROR_M | \
 			      VSC_SATA_INT_PHY_CHANGE),
+
+	/* Host private flags (hp_flags) */
+	VSC_SATA_HP_FLAG_MSI	= (1 << 0),
 };
 
+struct vsc_sata_host_priv {
+	u32			hp_flags;
+};
 
 #define is_vsc_sata_int_err(port_idx, int_status) \
 	 (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
@@ -118,6 +124,19 @@ static void vsc_sata_scr_write (struct a
 }
 
 
+static void vsc_sata_host_stop(struct ata_host_set *host_set)
+{
+	struct vsc_sata_host_priv *hpriv = host_set->private_data;
+	struct pci_dev *pdev = to_pci_dev(host_set->dev);
+
+	if (hpriv->hp_flags & VSC_SATA_HP_FLAG_MSI)
+		pci_disable_msi(pdev);
+	else
+		pci_intx(pdev, 0);
+	kfree (hpriv);
+	ata_pci_host_stop(host_set);
+}
+
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
 	void __iomem *mask_addr;
@@ -308,7 +327,7 @@ static const struct ata_port_operations 
 	.scr_write		= vsc_sata_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
+	.host_stop		= vsc_sata_host_stop,
 };
 
 static void __devinit vsc_sata_setup_port(struct ata_ioports *port, unsigned 
long base)
@@ -337,6 +356,7 @@ static int __devinit vsc_sata_init_one (
 {
 	static int printed_version;
 	struct ata_probe_ent *probe_ent = NULL;
+	struct vsc_sata_host_priv *hpriv;
 	unsigned long base;
 	int pci_dev_busy = 0;
 	void __iomem *mmio_base;
@@ -378,6 +398,7 @@ static int __devinit vsc_sata_init_one (
 		rc = -ENOMEM;
 		goto err_out_regions;
 	}
+
 	memset(probe_ent, 0, sizeof(*probe_ent));
 	probe_ent->dev = pci_dev_to_dev(pdev);
 	INIT_LIST_HEAD(&probe_ent->node);
@@ -389,19 +410,33 @@ static int __devinit vsc_sata_init_one (
 	}
 	base = (unsigned long) mmio_base;
 
+	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
+	if (!hpriv) {
+		rc = -ENOMEM;
+		goto err_out_iounmap;
+	}
+	memset(hpriv, 0, sizeof(*hpriv));
+
 	/*
 	 * Due to a bug in the chip, the default cache line size can't be used
 	 */
 	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
 
+	if (pci_enable_msi(pdev) == 0) {
+		hpriv->hp_flags |= VSC_SATA_HP_FLAG_MSI;
+		pci_intx(pdev, 0);
+	}
+	else
+		probe_ent->irq_flags = IRQF_SHARED;
+
 	probe_ent->sht = &vsc_sata_sht;
 	probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				ATA_FLAG_MMIO;
 	probe_ent->port_ops = &vsc_sata_ops;
 	probe_ent->n_ports = 4;
 	probe_ent->irq = pdev->irq;
-	probe_ent->irq_flags = IRQF_SHARED;
 	probe_ent->mmio_base = mmio_base;
+	probe_ent->private_data = hpriv;
 
 	/* We don't care much about the PIO/UDMA masks, but the core won't like us
 	 * if we don't fill these
@@ -428,10 +463,12 @@ static int __devinit vsc_sata_init_one (
 
 	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
-	kfree(probe_ent);
 
+	kfree(probe_ent);
 	return 0;
 
+err_out_iounmap:
+	pci_iounmap(pdev, mmio_base);
 err_out_free_ent:
 	kfree(probe_ent);
 err_out_regions:

Re: [PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

[Jeff Garzik]

daniel@wolstenholme.net wrote:
>> Wolstenholme, Daniel E wrote:
>>> Ok, hopefully I can get the attachment right here.
>>>
>>> Here's a resubmission of my MSI patch for the sata_vsc driver.
>>>
>>> I've added a new vsc_sata_host_priv data structure (basically copied
>>> from the sata_mv driver) which has an hp_flags bitfield with a bit to
>>> indicate whether MSI is being used. This is used in the new
>>> vsc_sata_host_stop() routine to call
>>> pci_disable_msi() if MSI is on, pci_intx() if not.
>>>
>>> Signed-off-by: Daniel Wolstenholme <daniel.e.wolstenholme@intel.com>
>> ACK the patch technical content.
>>
>> Unfortunately, the patch was corrupted, and tabs were converted to 
>> spaces, which makes it impossible to apply the patch.apply the patch.
> 
> Ok, this one should get it right.  This makes me miss the days of elm and no 
> firewalls.

Alas!

This attempt yielded:
> error: patch fragment without header at line 49: @@ -337,6 +356,7 @@ static int __devinit vsc_sata_init_one (
> error: patch fragment without header at line 57: @@ -378,6 +398,7 @@ static int __devinit vsc_sata_init_one (
> error: patch fragment without header at line 65: @@ -389,19 +410,33 @@ static int __devinit vsc_sata_init_one (
> error: patch fragment without header at line 100: @@ -428,10 +463,12 @@ static int __devinit vsc_sata_init_one (

Your previous attempt (MIME attachment) worked manually, but not through 
the normal git-applymbox merge mechanism than me (and others) use.

Also, your MIME attachment was based on kernel 2.6.18.  libata moves 
very rapidly, so you need to make sure that all your patches are based 
on the latest kernel, which is either kernel-2.6.X-rcY (in a Release 
Candidate cycle) or kernel-2.6.X-gitY (not -rc cycle).

Nonetheless, I managed to apply the patch (thank you git), and it is now 
queued for kernel 2.6.20 in repository jgarzik/libata-dev.git#upstream.

	Jeff

Re: [PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

[daniel]

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

>Wolstenholme, Daniel E wrote:
>> Ok, hopefully I can get the attachment right here.
>> 
>> Here's a resubmission of my MSI patch for the sata_vsc driver.
>> 
>> I've added a new vsc_sata_host_priv data structure (basically copied
>> from the sata_mv driver) which has an hp_flags bitfield with a bit to
>> indicate whether MSI is being used. This is used in the new
>> vsc_sata_host_stop() routine to call
>> pci_disable_msi() if MSI is on, pci_intx() if not.
>> 
>> Signed-off-by: Daniel Wolstenholme <daniel.e.wolstenholme@intel.com>
>
>ACK the patch technical content.
>
>Unfortunately, the patch was corrupted, and tabs were converted to 
>spaces, which makes it impossible to apply the patch.

Ok,
I'm going to try my personal webmail account now.  Sorry about all the 
trouble.

Dan


[-- Attachment #2: patch.sata_vsc --]
[-- Type: APPLICATION/OCTET-STREAM, Size: 3205 bytes --]

diff -uprN a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c	2006-09-19 20:42:06.000000000 -0700
+++ b/drivers/scsi/sata_vsc.c	2006-10-16 12:11:43.000000000 -0700
@@ -94,8 +94,14 @@ enum {
 			      VSC_SATA_INT_ERROR_P    | VSC_SATA_INT_ERROR_R | \
 			      VSC_SATA_INT_ERROR_E    | VSC_SATA_INT_ERROR_M | \
 			      VSC_SATA_INT_PHY_CHANGE),
+
+	/* Host private flags (hp_flags) */
+	VSC_SATA_HP_FLAG_MSI	= (1 << 0),
 };
 
+struct vsc_sata_host_priv {
+	u32			hp_flags;
+};
 
 #define is_vsc_sata_int_err(port_idx, int_status) \
 	 (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
@@ -118,6 +124,19 @@ static void vsc_sata_scr_write (struct a
 }
 
 
+static void vsc_sata_host_stop(struct ata_host_set *host_set)
+{
+	struct vsc_sata_host_priv *hpriv = host_set->private_data;
+	struct pci_dev *pdev = to_pci_dev(host_set->dev);
+
+	if (hpriv->hp_flags & VSC_SATA_HP_FLAG_MSI)
+		pci_disable_msi(pdev);
+	else
+		pci_intx(pdev, 0);
+	kfree (hpriv);
+	ata_pci_host_stop(host_set);
+}
+
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
 	void __iomem *mask_addr;
@@ -308,7 +327,7 @@ static const struct ata_port_operations 
 	.scr_write		= vsc_sata_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
+	.host_stop		= vsc_sata_host_stop,
 };
 
 static void __devinit vsc_sata_setup_port(struct ata_ioports *port, unsigned long base)
@@ -337,6 +356,7 @@ static int __devinit vsc_sata_init_one (
 {
 	static int printed_version;
 	struct ata_probe_ent *probe_ent = NULL;
+	struct vsc_sata_host_priv *hpriv;
 	unsigned long base;
 	int pci_dev_busy = 0;
 	void __iomem *mmio_base;
@@ -378,6 +398,7 @@ static int __devinit vsc_sata_init_one (
 		rc = -ENOMEM;
 		goto err_out_regions;
 	}
+
 	memset(probe_ent, 0, sizeof(*probe_ent));
 	probe_ent->dev = pci_dev_to_dev(pdev);
 	INIT_LIST_HEAD(&probe_ent->node);
@@ -389,19 +410,33 @@ static int __devinit vsc_sata_init_one (
 	}
 	base = (unsigned long) mmio_base;
 
+	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
+	if (!hpriv) {
+		rc = -ENOMEM;
+		goto err_out_iounmap;
+	}
+	memset(hpriv, 0, sizeof(*hpriv));
+
 	/*
 	 * Due to a bug in the chip, the default cache line size can't be used
 	 */
 	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
 
+	if (pci_enable_msi(pdev) == 0) {
+		hpriv->hp_flags |= VSC_SATA_HP_FLAG_MSI;
+		pci_intx(pdev, 0);
+	}
+	else
+		probe_ent->irq_flags = IRQF_SHARED;
+
 	probe_ent->sht = &vsc_sata_sht;
 	probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				ATA_FLAG_MMIO;
 	probe_ent->port_ops = &vsc_sata_ops;
 	probe_ent->n_ports = 4;
 	probe_ent->irq = pdev->irq;
-	probe_ent->irq_flags = IRQF_SHARED;
 	probe_ent->mmio_base = mmio_base;
+	probe_ent->private_data = hpriv;
 
 	/* We don't care much about the PIO/UDMA masks, but the core won't like us
 	 * if we don't fill these
@@ -428,10 +463,12 @@ static int __devinit vsc_sata_init_one (
 
 	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
-	kfree(probe_ent);
 
+	kfree(probe_ent);
 	return 0;
 
+err_out_iounmap:
+	pci_iounmap(pdev, mmio_base);
 err_out_free_ent:
 	kfree(probe_ent);
 err_out_regions:

[PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

[Wolstenholme, Daniel E]

Ok, hopefully I can get the attachment right here.

Here's a resubmission of my MSI patch for the sata_vsc driver.

I've added a new vsc_sata_host_priv data structure (basically copied
from the sata_mv driver) which has an hp_flags bitfield with a bit to
indicate whether MSI is being used. This is used in the new
vsc_sata_host_stop() routine to call
pci_disable_msi() if MSI is on, pci_intx() if not.

Signed-off-by: Daniel Wolstenholme <daniel.e.wolstenholme@intel.com>

--------

diff -uprN a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c 2006-09-19 20:42:06.000000000 -0700
+++ b/drivers/scsi/sata_vsc.c 2006-10-16 12:11:43.000000000 -0700
@@ -94,8 +94,14 @@ enum {
            VSC_SATA_INT_ERROR_P    | VSC_SATA_INT_ERROR_R | \
            VSC_SATA_INT_ERROR_E    | VSC_SATA_INT_ERROR_M | \
            VSC_SATA_INT_PHY_CHANGE),
+
+ /* Host private flags (hp_flags) */
+ VSC_SATA_HP_FLAG_MSI  = (1 << 0),
 };

+struct vsc_sata_host_priv {
+ u32     hp_flags;
+};

 #define is_vsc_sata_int_err(port_idx, int_status) \
   (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
@@ -118,6 +124,19 @@ static void vsc_sata_scr_write (struct a
 }


+static void vsc_sata_host_stop(struct ata_host_set *host_set)
+{
+ struct vsc_sata_host_priv *hpriv = host_set->private_data;
+ struct pci_dev *pdev = to_pci_dev(host_set->dev);
+
+ if (hpriv->hp_flags & VSC_SATA_HP_FLAG_MSI)
+   pci_disable_msi(pdev);
+ else
+   pci_intx(pdev, 0);
+ kfree (hpriv);
+ ata_pci_host_stop(host_set);
+}
+
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
  void __iomem *mask_addr;
@@ -308,7 +327,7 @@ static const struct ata_port_operations
  .scr_write    = vsc_sata_scr_write,
  .port_start   = ata_port_start,
  .port_stop    = ata_port_stop,
- .host_stop    = ata_pci_host_stop,
+ .host_stop    = vsc_sata_host_stop,
 };

 static void __devinit vsc_sata_setup_port(struct ata_ioports *port,
unsigned long base)
@@ -337,6 +356,7 @@ static int __devinit vsc_sata_init_one (
 {
  static int printed_version;
  struct ata_probe_ent *probe_ent = NULL;
+ struct vsc_sata_host_priv *hpriv;
  unsigned long base;
  int pci_dev_busy = 0;
  void __iomem *mmio_base;
@@ -378,6 +398,7 @@ static int __devinit vsc_sata_init_one (
    rc = -ENOMEM;
    goto err_out_regions;
  }
+
  memset(probe_ent, 0, sizeof(*probe_ent));
  probe_ent->dev = pci_dev_to_dev(pdev);
  INIT_LIST_HEAD(&probe_ent->node);
@@ -389,19 +410,33 @@ static int __devinit vsc_sata_init_one (
  }
  base = (unsigned long) mmio_base;

+ hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv) {
+   rc = -ENOMEM;
+   goto err_out_iounmap;
+ }
+ memset(hpriv, 0, sizeof(*hpriv));
+
  /*
   * Due to a bug in the chip, the default cache line size can't be used
   */
  pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);

+ if (pci_enable_msi(pdev) == 0) {
+   hpriv->hp_flags |= VSC_SATA_HP_FLAG_MSI;
+   pci_intx(pdev, 0);
+ }
+ else
+   probe_ent->irq_flags = IRQF_SHARED;
+
  probe_ent->sht = &vsc_sata_sht;
  probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
        ATA_FLAG_MMIO;
  probe_ent->port_ops = &vsc_sata_ops;
  probe_ent->n_ports = 4;
  probe_ent->irq = pdev->irq;
- probe_ent->irq_flags = IRQF_SHARED;
  probe_ent->mmio_base = mmio_base;
+ probe_ent->private_data = hpriv;

  /* We don't care much about the PIO/UDMA masks, but the core won't
like us
   * if we don't fill these
@@ -428,10 +463,12 @@ static int __devinit vsc_sata_init_one (

  /* FIXME: check ata_device_add return value */
  ata_device_add(probe_ent);
- kfree(probe_ent);

+ kfree(probe_ent);
  return 0;

+err_out_iounmap:
+ pci_iounmap(pdev, mmio_base);
 err_out_free_ent:
  kfree(probe_ent);
 err_out_regions:

Re: [PATCH] sata_vsc: Add MSI support to sata_vsc driver (resubmit) (fixed attachment)

[Jeff Garzik]

Wolstenholme, Daniel E wrote:
> Ok, hopefully I can get the attachment right here.
> 
> Here's a resubmission of my MSI patch for the sata_vsc driver.
> 
> I've added a new vsc_sata_host_priv data structure (basically copied
> from the sata_mv driver) which has an hp_flags bitfield with a bit to
> indicate whether MSI is being used. This is used in the new
> vsc_sata_host_stop() routine to call
> pci_disable_msi() if MSI is on, pci_intx() if not.
> 
> Signed-off-by: Daniel Wolstenholme <daniel.e.wolstenholme@intel.com>

ACK the patch technical content.

Unfortunately, the patch was corrupted, and tabs were converted to 
spaces, which makes it impossible to apply the patch.