[PATCH 1/2] ahci: disable correct irq for dymmy ports

[PATCH 1/2] ahci: disable correct irq for dymmy ports

[Christoph Hellwig]

irq already contains the interrupt number for the port, don't add the port
index to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 7461a58..dcf2c72 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2524,7 +2524,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 
 		/* Do not receive interrupts sent by dummy ports */
 		if (!pp) {
-			disable_irq(irq + i);
+			disable_irq(irq);
 			continue;
 		}
 
-- 
2.1.4

two ahci fixes from code inspection

[Christoph Hellwig]

Hi Tejun,

below are two things in ahci I found while converting the driver
to the new MSI / MSI-X vector allocator.

The first one looks like an obvious bug to me if we every have
these stray ports in a per-port IRQ vector setup.

The others one is more elaborate, but I think the per-port locking
is something we should do for the per-port MSI-X case just like for
the per-port MSI case.

[PATCH 1/2] ahci: disable correct irq for dymmy ports

[Christoph Hellwig]

irq already contains the interrupt number for the port, don't add the port
index to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 7461a58..dcf2c72 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2524,7 +2524,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 
 		/* Do not receive interrupts sent by dummy ports */
 		if (!pp) {
-			disable_irq(irq + i);
+			disable_irq(irq);
 			continue;
 		}
 
-- 
2.1.4

[PATCH 2/2] ahci: also use a per-port lock for the multi-MSIX case

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index dcf2c72..5a1329e 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2378,7 +2378,7 @@ static int ahci_port_start(struct ata_port *ap)
 	/*
 	 * Switch to per-port locking in case each port has its own MSI vector.
 	 */
-	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
+	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX)) {
 		spin_lock_init(&pp->lock);
 		ap->lock = &pp->lock;
 	}
-- 
2.1.4

Re: [PATCH 1/2] ahci: disable correct irq for dymmy ports

[Tejun Heo]

Hello, Christoph.

On Thu, Aug 11, 2016 at 07:26:01AM -0700, Christoph Hellwig wrote:
> irq already contains the interrupt number for the port, don't add the port
> index to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 7461a58..dcf2c72 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2524,7 +2524,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
>  
>  		/* Do not receive interrupts sent by dummy ports */
>  		if (!pp) {
> -			disable_irq(irq + i);
> +			disable_irq(irq);
>  			continue;
>  		}

I'm not following.  The driver at that point has irqs in the range
[@irq, @irq + @host->n_ports - 1] and is trying to activate them and
the above code block disables irq for a port which should be skipped.
Why wouldn't it need to be indexed?

Thanks.

-- 
tejun

Re: [PATCH 2/2] ahci: also use a per-port lock for the multi-MSIX case

[Tejun Heo]

On Thu, Aug 11, 2016 at 07:26:02AM -0700, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index dcf2c72..5a1329e 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2378,7 +2378,7 @@ static int ahci_port_start(struct ata_port *ap)
>  	/*
>  	 * Switch to per-port locking in case each port has its own MSI vector.
>  	 */
> -	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
> +	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX)) {

Looks good.  Applied to libata/for-4.9.

Thanks.

-- 
tejun

Re: [PATCH 1/2] ahci: disable correct irq for dymmy ports

[Christoph Hellwig]

On Thu, Aug 11, 2016 at 12:36:37PM -0400, Tejun Heo wrote:
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 7461a58..dcf2c72 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -2524,7 +2524,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
> >  
> >  		/* Do not receive interrupts sent by dummy ports */
> >  		if (!pp) {
> > -			disable_irq(irq + i);
> > +			disable_irq(irq);
> >  			continue;
> >  		}
> 
> I'm not following.  The driver at that point has irqs in the range
> [@irq, @irq + @host->n_ports - 1] and is trying to activate them and
> the above code block disables irq for a port which should be skipped.
> Why wouldn't it need to be indexed?

Because ahci_irq_vector does that indexing already.  This call changed
when Dan added ahci_irq_vector to support the MSI-X case - since
then ahci_irq_vector either does the base irq + index logic for
MSI or uses the msix_entries array for MSI-X, and irq contains the
result of that operation.

Re: [PATCH 1/2] ahci: disable correct irq for dymmy ports

[Tejun Heo]

On Thu, Aug 11, 2016 at 07:01:36PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 11, 2016 at 12:36:37PM -0400, Tejun Heo wrote:
> > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > > index 7461a58..dcf2c72 100644
> > > --- a/drivers/ata/libahci.c
> > > +++ b/drivers/ata/libahci.c
> > > @@ -2524,7 +2524,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
> > >  
> > >  		/* Do not receive interrupts sent by dummy ports */
> > >  		if (!pp) {
> > > -			disable_irq(irq + i);
> > > +			disable_irq(irq);
> > >  			continue;
> > >  		}
> > 
> > I'm not following.  The driver at that point has irqs in the range
> > [@irq, @irq + @host->n_ports - 1] and is trying to activate them and
> > the above code block disables irq for a port which should be skipped.
> > Why wouldn't it need to be indexed?
> 
> Because ahci_irq_vector does that indexing already.  This call changed
> when Dan added ahci_irq_vector to support the MSI-X case - since
> then ahci_irq_vector either does the base irq + index logic for
> MSI or uses the msix_entries array for MSI-X, and irq contains the
> result of that operation.

lol, sorry.  Was looking at an old (v4.0) tree.  Applying to
libata/for-4.8-fixes.

Thanks.

-- 
tejun