Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-10 Thread Hannes Reinecke 
On 11/10/2016 03:48 PM, Christoph Hellwig wrote:
> On Thu, Nov 10, 2016 at 07:48:20AM +0100, Hannes Reinecke wrote:
>> What I find quite irritating is that we still have to call
>> irq_set_affinity_hint(irq, NULL) when freeing up interrupts.
>> Can't we roll that into the call to free_irq() ?
> 
> If you do call it that's irritation, because you should not call
> irq_set_affinity_hint ever if you are using PCI_IRQ_AFFINITY, and
> the above branch doesn't call it.
> 
Ah. Sorry to have misread that.

>> And you _do_ have to setup a cpumap within the driver :-)
> 
> For the non-mq case, yes - but only to keep the functionality
> as-is.  We shouldn't add anything like this to a new driver.
> 
>> Anyway, remainder looks pretty close to what I've written up.
>> Feel free to add my 'Reviewed-by:' to it.
> 
> I'm mostly looking for someone who has the hardware to actually
> test it, though.
> 
Ah. Right.

Will give it a spin tomorrow.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-10 Thread Christoph Hellwig 
On Thu, Nov 10, 2016 at 07:48:20AM +0100, Hannes Reinecke wrote:
> What I find quite irritating is that we still have to call
> irq_set_affinity_hint(irq, NULL) when freeing up interrupts.
> Can't we roll that into the call to free_irq() ?

If you do call it that's irritation, because you should not call
irq_set_affinity_hint ever if you are using PCI_IRQ_AFFINITY, and
the above branch doesn't call it.

> And you _do_ have to setup a cpumap within the driver :-)

For the non-mq case, yes - but only to keep the functionality
as-is.  We shouldn't add anything like this to a new driver.

> Anyway, remainder looks pretty close to what I've written up.
> Feel free to add my 'Reviewed-by:' to it.

I'm mostly looking for someone who has the hardware to actually
test it, though.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-09 Thread Christoph Hellwig 
On Tue, Nov 08, 2016 at 04:02:54PM +0100, Hannes Reinecke wrote:
>> I've looked at the them but haven't started work, so feel free to go
>> ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.
>>
>> I've also started playing with lpfc, something I'll need to send to
>> you and James for testing and feedback soon.
>>
> Bah. Another duplicate then.
> Let's see who is faster :-)

Here are the lpfc patches - note that it's in a branch that merges
the recent irq affinity changes from the tip tree into the scsi tree
first, as we need updates from both of them:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/lpfc-msix
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-09 Thread Don Brace 

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Wednesday, November 09, 2016 9:45 AM
> To: Christoph Hellwig; Don Brace
> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; Hannes
> Reinecke
> Subject: Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors
> 
> EXTERNAL EMAIL
> 
> 
> On 11/09/2016 04:36 PM, Christoph Hellwig wrote:
> > On Wed, Nov 09, 2016 at 03:32:48PM +, Don Brace wrote:
> >> Do we need to add an entry for map_queues?
> >
> > The existing driver only supports a single submission queue.
> > If some of the devices support more than one submission queue
> > we could enhance the driver to support it, but we'd need docs
> > and hardware.
> >
> Hardware is not an issue :-)
> 
> However, I'm still curious about the layout of the queues.
> Are all queues (in performant mode) identical and can we use them for
> parallel submission? Or do we have to treat them differently?\

All of the submission queues are identical.

> (There were some rumours that it actually uses the queues for different
> I/O sizes ...)

This is an untrue rumor. :)

Thanks,
Don Brace

ESC - Smart Storage
Microsemi Corporation



> 
> Once that is cleared up it shouldn't be too hard moving to full
> multiqueue support.
> (If we have identical queues, that is :-)
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes ReineckeTeamlead Storage & Networking
> h...@suse.de   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-09 Thread Hannes Reinecke 

On 11/09/2016 04:36 PM, Christoph Hellwig wrote:

On Wed, Nov 09, 2016 at 03:32:48PM +, Don Brace wrote:

Do we need to add an entry for map_queues?


The existing driver only supports a single submission queue.
If some of the devices support more than one submission queue
we could enhance the driver to support it, but we'd need docs
and hardware.


Hardware is not an issue :-)

However, I'm still curious about the layout of the queues.
Are all queues (in performant mode) identical and can we use them for 
parallel submission? Or do we have to treat them differently?
(There were some rumours that it actually uses the queues for different 
I/O sizes ...)


Once that is cleared up it shouldn't be too hard moving to full 
multiqueue support.

(If we have identical queues, that is :-)

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-09 Thread Christoph Hellwig 
On Wed, Nov 09, 2016 at 03:32:48PM +, Don Brace wrote:
> Do we need to add an entry for map_queues?

The existing driver only supports a single submission queue.
If some of the devices support more than one submission queue
we could enhance the driver to support it, but we'd need docs
and hardware.

> Are you and Christoph still working on this patch? :)

I just need to post the version I have here after rebasing it.
Shouldn't take long, but I'm fighting a few fires at the moment,
hopefully later today.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-09 Thread Don Brace 
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Tuesday, November 08, 2016 1:12 AM
> To: Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; linux-scsi@vger.kernel.org; Hannes
> Reinecke; Hannes Reinecke; Don Brace
> Subject: [PATCH] hpsa: switch to pci_alloc_irq_vectors
> 
> EXTERNAL EMAIL
> 
> 
> Use pci_alloc_irq_vectors and drop the hand-crafted
> interrupt affinity routines.
> 
> Signed-off-by: Hannes Reinecke <h...@suse.com>
> Cc: Don Brace <don.br...@microsemi.com>

Do we need to add an entry for map_queues?

Are you and Christoph still working on this patch? :)

Thanks,
Don Brace

ESC - Smart Storage
Microsemi Corporation




> ---
>  drivers/scsi/hpsa.c | 72 
> +++--
>  drivers/scsi/hpsa.h |  1 -
>  2 files changed, 26 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 4e82b69..104e699 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -5618,7 +5618,7 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
> sh->sg_tablesize = h->maxsgentries;
> sh->transportt = hpsa_sas_transport_template;
> sh->hostdata[0] = (unsigned long) h;
> -   sh->irq = h->intr[h->intr_mode];
> +   sh->irq = pci_irq_vector(h->pdev, h->intr_mode);
> sh->unique_id = sh->irq;
> 
> h->scsi_host = sh;
> @@ -7667,14 +7667,9 @@ static void hpsa_disable_interrupt_mode(struct
> ctlr_info *h)
>   */
>  static void hpsa_interrupt_mode(struct ctlr_info *h)
>  {
> +   unsigned int irq_flags = PCI_IRQ_LEGACY;
>  #ifdef CONFIG_PCI_MSI
> -   int err, i;
> -   struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
> -
> -   for (i = 0; i < MAX_REPLY_QUEUES; i++) {
> -   hpsa_msix_entries[i].vector = 0;
> -   hpsa_msix_entries[i].entry = i;
> -   }
> +   int err;
> 
> /* Some boards advertise MSI but don't really support it */
> if ((h->board_id == 0x40700E11) || (h->board_id == 0x40800E11) ||
> @@ -7685,8 +7680,8 @@ static void hpsa_interrupt_mode(struct ctlr_info
> *h)
> h->msix_vector = MAX_REPLY_QUEUES;
> if (h->msix_vector > num_online_cpus())
> h->msix_vector = num_online_cpus();
> -   err = pci_enable_msix_range(h->pdev, hpsa_msix_entries,
> -   1, h->msix_vector);
> +   err = pci_alloc_irq_vectors(h->pdev, 1, h->msix_vector,
> +   PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
> if (err < 0) {
> dev_warn(>pdev->dev, "MSI-X init failed %d\n", 
> err);
> h->msix_vector = 0;
> @@ -7696,22 +7691,21 @@ static void hpsa_interrupt_mode(struct ctlr_info
> *h)
>"available\n", err);
> }
> h->msix_vector = err;
> -   for (i = 0; i < h->msix_vector; i++)
> -   h->intr[i] = hpsa_msix_entries[i].vector;
> return;
> }
>  single_msi_mode:
> if (pci_find_capability(h->pdev, PCI_CAP_ID_MSI)) {
> dev_info(>pdev->dev, "MSI capable controller\n");
> -   if (!pci_enable_msi(h->pdev))
> +   if (!pci_enable_msi(h->pdev)) {
> h->msi_vector = 1;
> -   else
> +   irq_flags = PCI_IRQ_MSI;
> +   } else
> dev_warn(>pdev->dev, "MSI init failed\n");
> }
>  default_int_mode:
>  #endif /* CONFIG_PCI_MSI */
> /* if we get here we're going to use the default interrupt mode */
> -   h->intr[h->intr_mode] = h->pdev->irq;
> +   pci_alloc_irq_vectors(h->pdev, 1, 1, irq_flags);
>  }
> 
>  static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
> @@ -8236,17 +8230,6 @@ static int hpsa_alloc_cmd_pool(struct ctlr_info *h)
> return -ENOMEM;
>  }
> 
> -static void hpsa_irq_affinity_hints(struct ctlr_info *h)
> -{
> -   int i, cpu;
> -
> -   cpu = cpumask_first(cpu_online_mask);
> -   for (i = 0; i < h->msix_vector; i++) {
> -   irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> -   cpu = cpumask_next(cpu, cpu_online_mask);
> -   }
> -}
> -
>  /* clear affinity hints and free MSI-X, MSI, or legacy INTx vector

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-08 Thread Hannes Reinecke 

On 11/08/2016 03:58 PM, Christoph Hellwig wrote:

On Tue, Nov 08, 2016 at 08:12:25AM +0100, Hannes Reinecke wrote:

Use pci_alloc_irq_vectors and drop the hand-crafted
interrupt affinity routines.


There are a couple more things we can do here.  I actually have a patch
in my tree that goes a little further, I'll post it in a bit.


Right. I'll wait for it.

If you also happen to have patches for megaraid and mpt3sas I'd be very 
much interested in looking into them; I'm currently working on 
converting them, too.


Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-08 Thread Christoph Hellwig 
On Tue, Nov 08, 2016 at 04:02:54PM +0100, Hannes Reinecke wrote:
>> I've also started playing with lpfc, something I'll need to send to
>> you and James for testing and feedback soon.
>>
> Bah. Another duplicate then.
> Let's see who is faster :-)

lpfc depends on the series adding the post_vetors support, so I can't
send it until we have that merged.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-08 Thread Hannes Reinecke 

On 11/08/2016 04:01 PM, Christoph Hellwig wrote:

On Tue, Nov 08, 2016 at 04:00:46PM +0100, Hannes Reinecke wrote:

Right. I'll wait for it.

If you also happen to have patches for megaraid and mpt3sas I'd be very
much interested in looking into them; I'm currently working on converting
them, too.


I've looked at the them but haven't started work, so feel free to go
ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.

I've also started playing with lpfc, something I'll need to send to
you and James for testing and feedback soon.


Bah. Another duplicate then.
Let's see who is faster :-)

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-08 Thread Christoph Hellwig 
On Tue, Nov 08, 2016 at 04:00:46PM +0100, Hannes Reinecke wrote:
> Right. I'll wait for it.
>
> If you also happen to have patches for megaraid and mpt3sas I'd be very 
> much interested in looking into them; I'm currently working on converting 
> them, too.

I've looked at the them but haven't started work, so feel free to go
ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.

I've also started playing with lpfc, something I'll need to send to
you and James for testing and feedback soon.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-08 Thread Christoph Hellwig 
On Tue, Nov 08, 2016 at 08:12:25AM +0100, Hannes Reinecke wrote:
> Use pci_alloc_irq_vectors and drop the hand-crafted
> interrupt affinity routines.

There are a couple more things we can do here.  I actually have a patch
in my tree that goes a little further, I'll post it in a bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] hpsa: switch to pci_alloc_irq_vectors

2016-11-07 Thread Hannes Reinecke 
Use pci_alloc_irq_vectors and drop the hand-crafted
interrupt affinity routines.

Signed-off-by: Hannes Reinecke 
Cc: Don Brace 
---
 drivers/scsi/hpsa.c | 72 +++--
 drivers/scsi/hpsa.h |  1 -
 2 files changed, 26 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4e82b69..104e699 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5618,7 +5618,7 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
sh->sg_tablesize = h->maxsgentries;
sh->transportt = hpsa_sas_transport_template;
sh->hostdata[0] = (unsigned long) h;
-   sh->irq = h->intr[h->intr_mode];
+   sh->irq = pci_irq_vector(h->pdev, h->intr_mode);
sh->unique_id = sh->irq;
 
h->scsi_host = sh;
@@ -7667,14 +7667,9 @@ static void hpsa_disable_interrupt_mode(struct ctlr_info 
*h)
  */
 static void hpsa_interrupt_mode(struct ctlr_info *h)
 {
+   unsigned int irq_flags = PCI_IRQ_LEGACY;
 #ifdef CONFIG_PCI_MSI
-   int err, i;
-   struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
-
-   for (i = 0; i < MAX_REPLY_QUEUES; i++) {
-   hpsa_msix_entries[i].vector = 0;
-   hpsa_msix_entries[i].entry = i;
-   }
+   int err;
 
/* Some boards advertise MSI but don't really support it */
if ((h->board_id == 0x40700E11) || (h->board_id == 0x40800E11) ||
@@ -7685,8 +7680,8 @@ static void hpsa_interrupt_mode(struct ctlr_info *h)
h->msix_vector = MAX_REPLY_QUEUES;
if (h->msix_vector > num_online_cpus())
h->msix_vector = num_online_cpus();
-   err = pci_enable_msix_range(h->pdev, hpsa_msix_entries,
-   1, h->msix_vector);
+   err = pci_alloc_irq_vectors(h->pdev, 1, h->msix_vector,
+   PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
if (err < 0) {
dev_warn(>pdev->dev, "MSI-X init failed %d\n", err);
h->msix_vector = 0;
@@ -7696,22 +7691,21 @@ static void hpsa_interrupt_mode(struct ctlr_info *h)
   "available\n", err);
}
h->msix_vector = err;
-   for (i = 0; i < h->msix_vector; i++)
-   h->intr[i] = hpsa_msix_entries[i].vector;
return;
}
 single_msi_mode:
if (pci_find_capability(h->pdev, PCI_CAP_ID_MSI)) {
dev_info(>pdev->dev, "MSI capable controller\n");
-   if (!pci_enable_msi(h->pdev))
+   if (!pci_enable_msi(h->pdev)) {
h->msi_vector = 1;
-   else
+   irq_flags = PCI_IRQ_MSI;
+   } else
dev_warn(>pdev->dev, "MSI init failed\n");
}
 default_int_mode:
 #endif /* CONFIG_PCI_MSI */
/* if we get here we're going to use the default interrupt mode */
-   h->intr[h->intr_mode] = h->pdev->irq;
+   pci_alloc_irq_vectors(h->pdev, 1, 1, irq_flags);
 }
 
 static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
@@ -8236,17 +8230,6 @@ static int hpsa_alloc_cmd_pool(struct ctlr_info *h)
return -ENOMEM;
 }
 
-static void hpsa_irq_affinity_hints(struct ctlr_info *h)
-{
-   int i, cpu;
-
-   cpu = cpumask_first(cpu_online_mask);
-   for (i = 0; i < h->msix_vector; i++) {
-   irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
-   cpu = cpumask_next(cpu, cpu_online_mask);
-   }
-}
-
 /* clear affinity hints and free MSI-X, MSI, or legacy INTx vectors */
 static void hpsa_free_irqs(struct ctlr_info *h)
 {
@@ -8255,15 +8238,13 @@ static void hpsa_free_irqs(struct ctlr_info *h)
if (!h->msix_vector || h->intr_mode != PERF_MODE_INT) {
/* Single reply queue, only one irq to free */
i = h->intr_mode;
-   irq_set_affinity_hint(h->intr[i], NULL);
-   free_irq(h->intr[i], >q[i]);
+   free_irq(pci_irq_vector(h->pdev, i), >q[i]);
h->q[i] = 0;
return;
}
 
for (i = 0; i < h->msix_vector; i++) {
-   irq_set_affinity_hint(h->intr[i], NULL);
-   free_irq(h->intr[i], >q[i]);
+   free_irq(pci_irq_vector(h->pdev, i), >q[i]);
h->q[i] = 0;
}
for (; i < MAX_REPLY_QUEUES; i++)
@@ -8288,17 +8269,18 @@ static int hpsa_request_irqs(struct ctlr_info *h,
/* If performant mode and MSI-X, use multiple reply queues */
for (i = 0; i < h->msix_vector; i++) {
sprintf(h->intrname[i], "%s-msix%d", h->devname, i);
-   rc = request_irq(h->intr[i], msixhandler,
-   0, h->intrname[i],
-