Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

[Thomas Gleixner]

On Tue, 19 Jul 2016, Eric Auger wrote:
>  /**
> + * msi_handle_doorbell_mappings: in case the irq data corresponds to an
> + * MSI that requires iommu mapping, traverse the irq domain hierarchy
> + * to retrieve the doorbells to handle and iommu_map/unmap them according
> + * to @map boolean.
> + *
> + * @data: irq data handle
> + * @map: mapping if true, unmapping if false
> + */


Please run that through the kernel doc generator. It does not work that way.

The format is:

/**
 * function_name - Short function description    
 * @arg1:	Description of arg1
 * @argument2:	Description of argument2
 *
 * Long explanation including documentation of the return values.
 */

> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
> +{
> +	const struct irq_chip_msi_doorbell_info *dbinfo;
> +	struct iommu_domain *domain;
> +	struct irq_chip *chip;
> +	struct device *dev;
> +	dma_addr_t iova;
> +	int ret = 0, cpu;
> +
> +	while (data) {
> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
> +		domain = iommu_msi_domain(dev);
> +		if (domain) {
> +			chip = irq_data_get_irq_chip(data);
> +			if (chip->msi_doorbell_info)
> +				break;
> +		}
> +		data = data->parent_data;
> +	}

Please split that out into a seperate function

struct irq_data *msi_get_doorbell_info(data)
{
	.....
		if (chip->msi_doorbell_info)
			return chip->msi_get_doorbell_info(data);
	}
	return NULL;
}

       info = msi_get_doorbell_info(data);
       .....

> +	if (!data)
> +		return 0;
> +
> +	dbinfo = chip->msi_doorbell_info(data);
> +	if (!dbinfo)
> +		return -EINVAL;
> +
> +	if (!dbinfo->doorbell_is_percpu) {
> +		if (!map) {
> +			iommu_msi_put_doorbell_iova(domain,
> +						    dbinfo->global_doorbell);
> +			return 0;
> +		}
> +		return iommu_msi_get_doorbell_iova(domain,
> +						   dbinfo->global_doorbell,
> +						   dbinfo->size, dbinfo->prot,
> +						   &iova);
> +	}

You can spare an indentation level with a helper function

    	if (!dbinfo->doorbell_is_percpu)
		return msi_map_global_doorbell(domain, dbinfo);

> +
> +	/* percpu doorbells */
> +	for_each_possible_cpu(cpu) {
> +		phys_addr_t __percpu *db_addr =
> +			per_cpu_ptr(dbinfo->percpu_doorbells, cpu);
> +
> +		if (!map) {
> +			iommu_msi_put_doorbell_iova(domain, *db_addr);
> +		} else {
> +
> +			ret = iommu_msi_get_doorbell_iova(domain, *db_addr,
> +							  dbinfo->size,
> +							  dbinfo->prot, &iova);
> +			if (ret)
> +				return ret;
> +		}
> +	}

Same here:

	for_each_possible_cpu(cpu) {
		ret = msi_map_percpu_doorbell(domain, cpu);
		if (ret)
			return ret;
	}
     	return 0;
     
Hmm?

> +
> +	return 0;
> +}
> +
> +/**
>   * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
>   * @domain:	The domain to allocate from
>   * @dev:	Pointer to device struct of the device for which the interrupts
> @@ -352,17 +423,29 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  
>  		virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used,
>  					       dev_to_node(dev), &arg, false);
> -		if (virq < 0) {
> -			ret = -ENOSPC;
> -			if (ops->handle_error)
> -				ret = ops->handle_error(domain, desc, ret);
> -			if (ops->msi_finish)
> -				ops->msi_finish(&arg, ret);
> -			return ret;
> -		}
> +		if (virq < 0)
> +			goto error;
>  
>  		for (i = 0; i < desc->nvec_used; i++)
>  			irq_set_msi_desc_off(virq, i, desc);
> +
> +		for (i = 0; i < desc->nvec_used; i++) {
> +			struct irq_data *d = irq_get_irq_data(virq + i);
> +
> +			ret = msi_handle_doorbell_mappings(d, true);
> +			if (ret)
> +				break;
> +		}
> +		if (ret) {
> +			for (; i >= 0; i--) {
> +				struct irq_data *d = irq_get_irq_data(virq + i);
> +
> +				msi_handle_doorbell_mappings(d, false);
> +			}
> +			irq_domain_free_irqs(virq, desc->nvec_used);
> +			desc->irq = 0;
> +			goto error;

How is that supposed to work? You clear desc->irq and then you call
ops->handle_error.

Why are you adding this extra stuff here? Look at the call sites of
msi_domain_alloc_irqs(). All of them use msi_domain_free_irqs() in case of
error. There is no reason why you can't do the same....

>  /**
> @@ -396,6 +486,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
>  		 * entry. If that's the case, don't do anything.
>  		 */
>  		if (desc->irq) {
> +			msi_handle_doorbell_mappings(
> +				irq_get_irq_data(desc->irq),
> +				false);
>  			irq_domain_free_irqs(desc->irq, desc->nvec_used);
>  			desc->irq = 0;

Can you please restructure the code so it reads

		if (desc->irq)
			continue;

		msi_handle_doorbell_mappings(irq_get_irq_data(desc->irq),
					     false);	
		irq_domain_free_irqs(desc->irq, desc->nvec_used);
		desc->irq = 0;

Just blindly whacking stuff into the 80 char limit is not helping readability.

Thanks,

	tglx