Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450486 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 11:10 +0200
    Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Auger Eric <eric.auger@redhat.com> - 2016-07-26 12:00 +0200
      Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on  msi_domain_alloc/free_irqs Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 13:10 +0200

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

B1;2802;0cEric,

On Mon, 25 Jul 2016, Auger Eric wrote:
> On 20/07/2016 11:04, Thomas Gleixner wrote:
> > On Tue, 19 Jul 2016, Eric Auger wrote:
> >> +		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.

The logic here is: If the allocation does not succeed for the requested number
of interrupts, we tell the caller how many interrupts we were able to set up.
So the caller can decide what to do.

In your case you don't want to have a partial allocation, so instead of
playing silly games with desc->irq you should add a flag which tells the PCI
code that you are not interested in a partial allocation and that it should
return an error code instead.

Something like PCI_DEV_FLAGS_MSI_NO_PARTIAL_ALLOC should do the trick.

> As for the irq_domain_free_irqs I think I can remove it since handled later.

Not only the free_irqs(). You should let the teardown function handle
everything including your doorbell mapping teardown. It's nothing special and
free_msi_irqs() at the end of msix_capability_init() will take care of it.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1450511

Hi Thomas,

On 26/07/2016 11:00, Thomas Gleixner wrote:
> B1;2802;0cEric,
> 
> On Mon, 25 Jul 2016, Auger Eric wrote:
>> On 20/07/2016 11:04, Thomas Gleixner wrote:
>>> On Tue, 19 Jul 2016, Eric Auger wrote:
>>>> +		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.
> 
> The logic here is: If the allocation does not succeed for the requested number
> of interrupts, we tell the caller how many interrupts we were able to set up.
> So the caller can decide what to do.
> 
> In your case you don't want to have a partial allocation, so instead of
> playing silly games with desc->irq you should add a flag which tells the PCI
> code that you are not interested in a partial allocation and that it should
> return an error code instead.

In that case can we consider we even succeeded in allocating 1 MSI? In case the
IOMMU mapping fails, the MSI transaction will never reach the target MSI frame
so it is not usable. So when you mean "partial" I understand we did not succeed
in allocating maxvec IRQs, correct? Here we succeeded in allocating 0 IRQ and still
msi_capability_init returns 1.

msi_capability_init doc-comment says "a positive return value indicates the number of
interrupts which could have been allocated."

I understand allocation success currently only depends on the fact virq was allocated
and set to desc->irq. But with that IOMMU stuff doesn't the criteria changes?


> Something like PCI_DEV_FLAGS_MSI_NO_PARTIAL_ALLOC should do the trick.
> 
>> As for the irq_domain_free_irqs I think I can remove it since handled later.
> 
> Not only the free_irqs(). You should let the teardown function handle
> everything including your doorbell mapping teardown. It's nothing special and
> free_msi_irqs() at the end of msix_capability_init() will take care of it.
Yep I was forced to call free_irqs myself since free_msi_irqs was doing nothing
due the fact I resetted the irq field. Wrong thing loop ;-)

Thanks

Eric

> 
> Thanks,
> 
> 	tglx
> 

[toc] | [prev] | [next] | [standalone]

#1450532

On Tue, 26 Jul 2016, Auger Eric wrote:
> On 26/07/2016 11:00, Thomas Gleixner wrote:
> > In your case you don't want to have a partial allocation, so instead of
> > playing silly games with desc->irq you should add a flag which tells the PCI
> > code that you are not interested in a partial allocation and that it should
> > return an error code instead.
> 

> In that case can we consider we even succeeded in allocating 1 MSI? In case
> the IOMMU mapping fails, the MSI transaction will never reach the target MSI
> frame so it is not usable. So when you mean "partial" I understand we did
> not succeed in allocating maxvec IRQs, correct? Here we succeeded in
> allocating 0 IRQ and still msi_capability_init returns 1.
>
> msi_capability_init doc-comment says "a positive return value indicates the
> number of interrupts which could have been allocated."
> 
> I understand allocation success currently only depends on the fact virq was
> allocated and set to desc->irq. But with that IOMMU stuff doesn't the
> criteria changes?

Right. But then you need to express it differently in a consistent way. Not by
hacking around it by setting desc->irq to 0.

Something like a flag field in msi_desc which denotes various properties would
be a possible solution. MSI_IRQ_ALLOCATED and MSI_IRQ_REMAPPED would be
sufficient for now. And the deallocation/cleanup would rely on those flags
rather than checking desc->irq.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web