Hi Thomas,
On 20/07/2016 11:04, Thomas Gleixner wrote:
> 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.
if I don't clear the desc->irq I enter an infinite loop in
pci_enable_msix_range.
This happens because msix_capability_init and pcie_enable_msix returns 1.
In msix_capability_init, at out_avail: we enumerate the msi_desc which have a
non
zero irq, hence the returned value equal to 1.
Currently the only handle_error ops I found, pci_msi_domain_handle_error does
not
use irq field so works although questionable.
As for the irq_domain_free_irqs I think I can remove it since handled later.
How do you advise to handle the above situation?
Thanks
Eric
>
> 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
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
