Xen devel - Sep 2012 - [PATCH] VT-d: split .ack and .disable DMA-MSI actors

Calling irq_complete_move() from .disable is wrong, breaking S3 resume.

Comparing with all other .ack actors, it was also missing a call to
move_{native,masked}_irq(). As the actor is masking its interrupt
anyway (albeit it''s not immediately obvious why), the latter is the
better choice.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1039,8 +1039,6 @@ static void dma_msi_mask(struct irq_desc
     unsigned long flags;
     struct iommu *iommu = desc->action->dev_id;
 
-    irq_complete_move(desc);
-
     /* mask it */
     spin_lock_irqsave(&iommu->register_lock, flags);
     dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
@@ -1053,6 +1051,13 @@ static unsigned int dma_msi_startup(stru
     return 0;
 }
 
+static void dma_msi_ack(struct irq_desc *desc)
+{
+    irq_complete_move(desc);
+    dma_msi_mask(desc);
+    move_masked_irq(desc);
+}
+
 static void dma_msi_end(struct irq_desc *desc, u8 vector)
 {
     dma_msi_unmask(desc);
@@ -1114,7 +1119,7 @@ static hw_irq_controller dma_msi_type = 
     .shutdown = dma_msi_mask,
     .enable = dma_msi_unmask,
     .disable = dma_msi_mask,
-    .ack = dma_msi_mask,
+    .ack = dma_msi_ack,
     .end = dma_msi_end,
     .set_affinity = dma_msi_set_affinity,
 };





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 07/09/2012 13:45, "Jan Beulich" <JBeulich@suse.com> wrote:
> Calling irq_complete_move() from .disable is wrong, breaking S3 resume.
> 
> Comparing with all other .ack actors, it was also missing a call to
> move_{native,masked}_irq(). As the actor is masking its interrupt
> anyway (albeit it''s not immediately obvious why), the latter is
the
> better choice.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
As far as I understand it
Acked-by: Keir Fraser <keir@xen.org>

I guess you are looking for an Intel ack as well.

 -- Keir
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1039,8 +1039,6 @@ static void dma_msi_mask(struct irq_desc
>      unsigned long flags;
>      struct iommu *iommu = desc->action->dev_id;
>  
> -    irq_complete_move(desc);
> -
>      /* mask it */
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
> @@ -1053,6 +1051,13 @@ static unsigned int dma_msi_startup(stru
>      return 0;
>  }
>  
> +static void dma_msi_ack(struct irq_desc *desc)
> +{
> +    irq_complete_move(desc);
> +    dma_msi_mask(desc);
> +    move_masked_irq(desc);
> +}
> +
>  static void dma_msi_end(struct irq_desc *desc, u8 vector)
>  {
>      dma_msi_unmask(desc);
> @@ -1114,7 +1119,7 @@ static hw_irq_controller dma_msi_type >     
.shutdown = dma_msi_mask,
>      .enable = dma_msi_unmask,
>      .disable = dma_msi_mask,
> -    .ack = dma_msi_mask,
> +    .ack = dma_msi_ack,
>      .end = dma_msi_end,
>      .set_affinity = dma_msi_set_affinity,
>  };
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 07.09.12 at 17:05, Keir Fraser <keir.xen@gmail.com> wrote:
> On 07/09/2012 13:45, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> Calling irq_complete_move() from .disable is wrong, breaking S3 resume.
>> 
>> Comparing with all other .ack actors, it was also missing a call to
>> move_{native,masked}_irq(). As the actor is masking its interrupt
>> anyway (albeit it''s not immediately obvious why), the latter
is the
>> better choice.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> As far as I understand it
> Acked-by: Keir Fraser <keir@xen.org>
> 
> I guess you are looking for an Intel ack as well.
Yes, that''s why I Cc-ed Xiantao.

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, September 07, 2012 11:52 PM
> To: Keir Fraser; xen-devel
> Cc: Zhang, Xiantao
> Subject: Re: [Xen-devel] [PATCH] VT-d: split .ack and .disable DMA-MSI
> actors
> 
> >>> On 07.09.12 at 17:05, Keir Fraser <keir.xen@gmail.com>
wrote:
> > On 07/09/2012 13:45, "Jan Beulich" <JBeulich@suse.com>
wrote:
> >
> >> Calling irq_complete_move() from .disable is wrong, breaking S3
resume.
> >>
> >> Comparing with all other .ack actors, it was also missing a call
to
> >> move_{native,masked}_irq(). As the actor is masking its interrupt
> >> anyway (albeit it''s not immediately obvious why), the
latter is the
> >> better choice.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > As far as I understand it
> > Acked-by: Keir Fraser <keir@xen.org>
> >
> > I guess you are looking for an Intel ack as well.
> 
> Yes, that''s why I Cc-ed Xiantao.
Thanks for the fix!  
Acked-by Xiantao Zhang <xiantao.zhang@intel.com>!  
Xiantao