Re: [patch 2/2] Intel IOMMU Suspend/Resume Support for Queued Invalidation.

[Ingo Molnar <mingo@elte.hu>]

(only small style nits.)

* fenghua.yu@intel.com <fenghua.yu@intel.com> wrote:

>  /*
> + * Enable queued invalidation.
> + */
> +static void __dmar_enable_qi(struct intel_iommu *iommu)

nice helper function.

> +{
> +	u32 cmd, sts;
> +	unsigned long flags;
> +	struct q_inval *qi = iommu->qi;
> +
> +	qi->free_head = qi->free_tail = 0;
> +	qi->free_cnt = QI_LENGTH;
> +
> +	spin_lock_irqsave(&iommu->register_lock, flags);
> +	/* write zero to the tail reg */
> +	writel(0, iommu->reg + DMAR_IQT_REG);

( small nit: critical sections are important to see at a glance, and 
  they are easier to recognize if there's a newline before and after 
  the spin_lock_irqsave() call. That will also make the comment 
  after that stand out more. )

> +
> +	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
> +
> +	cmd = iommu->gcmd | DMA_GCMD_QIE;
> +	iommu->gcmd |= DMA_GCMD_QIE;
> +	writel(cmd, iommu->reg + DMAR_GCMD_REG);
> +
> +	/* Make sure hardware complete it */
> +	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
> +	spin_unlock_irqrestore(&iommu->register_lock, flags);

(ditto.)

> +}
> +
> +/*
>   * Enable Queued Invalidation interface. This is a must to support
>   * interrupt-remapping. Also used by DMA-remapping, which replaces
>   * register based IOTLB invalidation.
>   */
>  int dmar_enable_qi(struct intel_iommu *iommu)
>  {
> -	u32 cmd, sts;
> -	unsigned long flags;
>  	struct q_inval *qi;
>  
>  	if (!ecap_qis(iommu->ecap))
> @@ -835,19 +860,7 @@ int dmar_enable_qi(struct intel_iommu *iommu)
>  
>  	spin_lock_init(&qi->q_lock);
>  
> -	spin_lock_irqsave(&iommu->register_lock, flags);
> -	/* write zero to the tail reg */
> -	writel(0, iommu->reg + DMAR_IQT_REG);
> -
> -	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
> -
> -	cmd = iommu->gcmd | DMA_GCMD_QIE;
> -	iommu->gcmd |= DMA_GCMD_QIE;
> -	writel(cmd, iommu->reg + DMAR_GCMD_REG);
> -
> -	/* Make sure hardware complete it */
> -	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
> -	spin_unlock_irqrestore(&iommu->register_lock, flags);
> +	__dmar_enable_qi(iommu);
>  
>  	return 0;
>  }
> @@ -1102,3 +1115,27 @@ int __init enable_drhd_fault_handling(void)
>  
>  	return 0;
>  }
> +
> +/*
> + * Re-enable Queued Invalidation interface.
> + */
> +int dmar_reenable_qi(struct intel_iommu *iommu)
> +{
> +	if (!ecap_qis(iommu->ecap))
> +		return -ENOENT;
> +
> +	if (!iommu->qi)
> +		return -ENOENT;

> +
> +	/*
> +	 * First disable queued invalidation.
> +	 */
> +	dmar_disable_qi(iommu);
> +	/* Then enable queued invalidation again. Since there is no pending
> +	 * invalidation requests now, it's safe to re-enable queued
> +	 * invalidation.
> +	 */
> +	__dmar_enable_qi(iommu);

the comment style looks a bit inconsistent here - the second 
one should be full winged comments as well, i.e.:

> +	/*
> +	 * First disable queued invalidation.
> +	 */
> +	dmar_disable_qi(iommu);
> +
> +	/*
> +	 * Then enable queued invalidation again. Since there is no pending
> +	 * invalidation requests now, it's safe to re-enable queued
> +	 * invalidation.
> +	 */
> +	__dmar_enable_qi(iommu);

Thanks,

	Ingo