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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
