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

Groups > linux.kernel > #1621400 > unrolled thread

Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge

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: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-11 17:20 +0200
    Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-04-11 18:00 +0200
      Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-11 18:30 +0200
        Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-04-11 18:50 +0200
          Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-11 20:00 +0200
            Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-04-12 10:10 +0200
              Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-04-12 12:00 +0200
                Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-19 13:30 +0200
                  Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-20 10:30 +0200
                    Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Marc Zyngier <marc.zyngier@arm.com> - 2017-04-20 11:50 +0200
              Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge Mason <slash.tmp@free.fr> - 2017-04-12 12:00 +0200

#1621400 — Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge

On 27/03/2017 19:09, Marc Zyngier wrote:

> Here's what your system looks like:
> 
> PCI-EP -------> MSI Controller ------> INTC
>          MSI                    IRQ
> 
> A PCI MSI is always edge. No ifs, no buts. That's what it is, and nothing
> else. Now, your MSI controller signals its output using a level interrupt,
> since you need to whack it on the head so that it lowers its line.
> 
> There is not a single trigger, because there is not a single interrupt.

Hello Marc,

I was hoping you or Thomas might help clear some confusion
in my mind around IRQ domains (struct irq_domain).

I have read https://www.kernel.org/doc/Documentation/IRQ-domain.txt

IIUC, there should be one IRQ domain per IRQ controller.

I have this MSI controller handling 256 interrupts, so I should
have *one* domain for all possible MSIs. Yet the Altera driver
registers *two* domains (msi_domain and inner_domain).

Could I make everything work with a single IRQ domain?

I'm confused by the sequence:

  irq_dom = irq_domain_create_linear(fwnode, MSI_COUNT, &msi_dom_ops, pcie);
  msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);

It seems I should be able to call only pci_msi_create_irq_domain...
And the parent of the MSI controller is given in the DT.

I'm not quite sure how I tell pci_msi_create_irq_domain how many
MSIs it's supposed to manage. Nor how I pass my private struct.

I will keep looking.

Regards.

[toc] | [next] | [standalone]

#1621429

On 11/04/17 16:13, Mason wrote:
> On 27/03/2017 19:09, Marc Zyngier wrote:
> 
>> Here's what your system looks like:
>>
>> PCI-EP -------> MSI Controller ------> INTC
>>          MSI                    IRQ
>>
>> A PCI MSI is always edge. No ifs, no buts. That's what it is, and nothing
>> else. Now, your MSI controller signals its output using a level interrupt,
>> since you need to whack it on the head so that it lowers its line.
>>
>> There is not a single trigger, because there is not a single interrupt.
> 
> Hello Marc,
> 
> I was hoping you or Thomas might help clear some confusion
> in my mind around IRQ domains (struct irq_domain).
> 
> I have read https://www.kernel.org/doc/Documentation/IRQ-domain.txt
> 
> IIUC, there should be one IRQ domain per IRQ controller.
> 
> I have this MSI controller handling 256 interrupts, so I should
> have *one* domain for all possible MSIs. Yet the Altera driver
> registers *two* domains (msi_domain and inner_domain).
> 
> Could I make everything work with a single IRQ domain?

No, because you have two irqchips. One that deals with the HW, and the
other that deals with the MSIs how they are presented to the kernel,
depending on the bus (PCI or something else). The fact that it doesn't
really drive any HW doesn't make it irrelevant.

> I'm confused by the sequence:
> 
>   irq_dom = irq_domain_create_linear(fwnode, MSI_COUNT, &msi_dom_ops, pcie);
>   msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
> 
> It seems I should be able to call only pci_msi_create_irq_domain...
> And the parent of the MSI controller is given in the DT.

No, see above.

> I'm not quite sure how I tell pci_msi_create_irq_domain how many
> MSIs it's supposed to manage. Nor how I pass my private struct.

You don't need to tell it anything about the number of interrupts you
manage. As for your private structure, you've already given it to your
low level domain, and there is no need to propagate it any further.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1621455

On 11/04/2017 17:49, Marc Zyngier wrote:
> On 11/04/17 16:13, Mason wrote:
>> On 27/03/2017 19:09, Marc Zyngier wrote:
>>
>>> Here's what your system looks like:
>>>
>>> PCI-EP -------> MSI Controller ------> INTC
>>>          MSI                    IRQ
>>>
>>> A PCI MSI is always edge. No ifs, no buts. That's what it is, and nothing
>>> else. Now, your MSI controller signals its output using a level interrupt,
>>> since you need to whack it on the head so that it lowers its line.
>>>
>>> There is not a single trigger, because there is not a single interrupt.
>>
>> Hello Marc,
>>
>> I was hoping you or Thomas might help clear some confusion
>> in my mind around IRQ domains (struct irq_domain).
>>
>> I have read https://www.kernel.org/doc/Documentation/IRQ-domain.txt
>>
>> IIUC, there should be one IRQ domain per IRQ controller.
>>
>> I have this MSI controller handling 256 interrupts, so I should
>> have *one* domain for all possible MSIs. Yet the Altera driver
>> registers *two* domains (msi_domain and inner_domain).
>>
>> Could I make everything work with a single IRQ domain?
> 
> No, because you have two irqchips. One that deals with the HW, and the
> other that deals with the MSIs how they are presented to the kernel,
> depending on the bus (PCI or something else). The fact that it doesn't
> really drive any HW doesn't make it irrelevant.

The example given in IRQ-domain.txt is

  Device --> IOAPIC -> Interrupt remapping Controller -> Local APIC -> CPU

with an irq_domain for each interrupt controller.


On my system I have:

  PCI-EP -> MSI controller -> System INTC -> GIC -> CPU

The driver for System INTC is drivers/irqchip/irq-tango.c
I think it has only one domain.

For the GIC, drivers/irqchip/irq-gic.c
I see a call to irq_domain_create_linear()

Is the handling of MSI different, and that is why we need
two domains? (Sorry, I did not understand that part well.)

When I looked at drivers/pci/host/pci-hyperv.c
they seem to have a single pci_msi_create_irq_domain call,
no call to domain_add or domain_create.
And they have a single struct irq_chip.

> You don't need to tell it anything about the number of interrupts you
> manage. As for your private structure, you've already given it to your
> low level domain, and there is no need to propagate it any further.

My main issue is that in the ack callback, I was in the "wrong"
domain, in that d->hwirq was not the MSI number. So I thought
I needed a single irq_domain.

Is there a function to map virq to the hwirq in any domain?

Regards.

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

#1621471

On 11/04/17 17:26, Mason wrote:
> On 11/04/2017 17:49, Marc Zyngier wrote:
>> On 11/04/17 16:13, Mason wrote:
>>> On 27/03/2017 19:09, Marc Zyngier wrote:
>>>
>>>> Here's what your system looks like:
>>>>
>>>> PCI-EP -------> MSI Controller ------> INTC
>>>>          MSI                    IRQ
>>>>
>>>> A PCI MSI is always edge. No ifs, no buts. That's what it is, and nothing
>>>> else. Now, your MSI controller signals its output using a level interrupt,
>>>> since you need to whack it on the head so that it lowers its line.
>>>>
>>>> There is not a single trigger, because there is not a single interrupt.
>>>
>>> Hello Marc,
>>>
>>> I was hoping you or Thomas might help clear some confusion
>>> in my mind around IRQ domains (struct irq_domain).
>>>
>>> I have read https://www.kernel.org/doc/Documentation/IRQ-domain.txt
>>>
>>> IIUC, there should be one IRQ domain per IRQ controller.
>>>
>>> I have this MSI controller handling 256 interrupts, so I should
>>> have *one* domain for all possible MSIs. Yet the Altera driver
>>> registers *two* domains (msi_domain and inner_domain).
>>>
>>> Could I make everything work with a single IRQ domain?
>>
>> No, because you have two irqchips. One that deals with the HW, and the
>> other that deals with the MSIs how they are presented to the kernel,
>> depending on the bus (PCI or something else). The fact that it doesn't
>> really drive any HW doesn't make it irrelevant.
> 
> The example given in IRQ-domain.txt is
> 
>   Device --> IOAPIC -> Interrupt remapping Controller -> Local APIC -> CPU
> 
> with an irq_domain for each interrupt controller.

Which doesn't use the generic MSI layer the way arm/arm64 do, so that's
the wrong example.

> 
> 
> On my system I have:
> 
>   PCI-EP -> MSI controller -> System INTC -> GIC -> CPU
> 
> The driver for System INTC is drivers/irqchip/irq-tango.c
> I think it has only one domain.
> 
> For the GIC, drivers/irqchip/irq-gic.c
> I see a call to irq_domain_create_linear()

Can we please stick to the problem at hand and not drift into other
considerations which do not matter at all?

> Is the handling of MSI different, and that is why we need
> two domains? (Sorry, I did not understand that part well.)

Let me repeat it again, then:
- You have a top-level MSI domain that is completely virtual, mapping a
virtual hwirq to the virtual interrupt. Nothing to see here.
- You have your own irqdomain, associated with your own irq_chip, which
does what it needs to do talking to the HW and allocating interrupts.

> When I looked at drivers/pci/host/pci-hyperv.c
> they seem to have a single pci_msi_create_irq_domain call,
> no call to domain_add or domain_create.
> And they have a single struct irq_chip.

Which is not using the generic MSI layer the way we do either.

> 
>> You don't need to tell it anything about the number of interrupts you
>> manage. As for your private structure, you've already given it to your
>> low level domain, and there is no need to propagate it any further.
> 
> My main issue is that in the ack callback, I was in the "wrong"
> domain, in that d->hwirq was not the MSI number. So I thought
> I needed a single irq_domain.

No. You need two, but you only need to manage yours.

> Is there a function to map virq to the hwirq in any domain?

Be more precise. If you want the hwirq associated with the view of a
virq in a given domain, that's the hwirq field in the corresponding
irq_data structure. Or are you after something else?

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1621508

On 11/04/2017 18:43, Marc Zyngier wrote:

> On 11/04/17 17:26, Mason wrote:
>
>> Is there a function to map virq to the hwirq in any domain?
> 
> Be more precise. If you want the hwirq associated with the view of a
> virq in a given domain, that's the hwirq field in the corresponding
> irq_data structure. Or are you after something else?

I registered an unmask method for my irq_chip.
(IIUC, I'm supposed to unmask a specific MSI in this callback.)

# cat /proc/interrupts 
           CPU0       CPU1       
 30:          0          0    MSIfoo      0 Edge      aerdrv
 34:          0          0    MSIfoo 524288 Edge      xhci_hcd
 35:          0          0    MSIfoo 524289 Edge      xhci_hcd
 36:          0          0    MSIfoo 524290 Edge      xhci_hcd


void foo_unmask(struct irq_data *data)
{
	int xx,yy;
	struct irq_domain *dom = data->domain;
	printk("%s: irq_data=%p irq=%u hwirq=%lu chip=%p dom=%p pdata=%p data=%p\n",
		__func__, data, data->irq, data->hwirq, data->chip, data->domain,
		data->parent_data, data->chip_data);
	printk("%s ops=%p data=%p parent=%p\n\n",
		dom->name, dom->ops, dom->host_data, dom->parent);
	printk("pcie=%p\n", dom->parent->host_data);
	//dump_stack();
	pci_msi_unmask_irq(data);
	struct tango_pcie *pcie = data->domain->parent->host_data;
	printk("\n%s: pcie=%p irq=%u hwirq=%lu\n\n",
		__func__, pcie, data->irq, data->hwirq);
	xx = irq_find_mapping(pcie->irq_domain, data->hwirq);
	yy = irq_find_mapping(pcie->msi_domain, data->hwirq);
	printk("xx=%d yy=%d\n", xx, yy);
}

so data->irq is the virq (30, 34, 35, 36)
and data->hwirq is the domain hwirq (0, 524288, 524289, 524290)

Is there a way to map hwirq 524288 to MSI 0, hwirq 524289 to MSI 1, etc?

Regards.

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

#1621904

On 11/04/17 18:52, Mason wrote:
> On 11/04/2017 18:43, Marc Zyngier wrote:
> 
>> On 11/04/17 17:26, Mason wrote:
>>
>>> Is there a function to map virq to the hwirq in any domain?
>>
>> Be more precise. If you want the hwirq associated with the view of a
>> virq in a given domain, that's the hwirq field in the corresponding
>> irq_data structure. Or are you after something else?
> 
> I registered an unmask method for my irq_chip.
> (IIUC, I'm supposed to unmask a specific MSI in this callback.)
> 
> # cat /proc/interrupts 
>            CPU0       CPU1       
>  30:          0          0    MSIfoo      0 Edge      aerdrv
>  34:          0          0    MSIfoo 524288 Edge      xhci_hcd
>  35:          0          0    MSIfoo 524289 Edge      xhci_hcd
>  36:          0          0    MSIfoo 524290 Edge      xhci_hcd
> 
> 
> void foo_unmask(struct irq_data *data)
> {
> 	int xx,yy;
> 	struct irq_domain *dom = data->domain;
> 	printk("%s: irq_data=%p irq=%u hwirq=%lu chip=%p dom=%p pdata=%p data=%p\n",
> 		__func__, data, data->irq, data->hwirq, data->chip, data->domain,
> 		data->parent_data, data->chip_data);
> 	printk("%s ops=%p data=%p parent=%p\n\n",
> 		dom->name, dom->ops, dom->host_data, dom->parent);
> 	printk("pcie=%p\n", dom->parent->host_data);
> 	//dump_stack();
> 	pci_msi_unmask_irq(data);
> 	struct tango_pcie *pcie = data->domain->parent->host_data;
> 	printk("\n%s: pcie=%p irq=%u hwirq=%lu\n\n",
> 		__func__, pcie, data->irq, data->hwirq);
> 	xx = irq_find_mapping(pcie->irq_domain, data->hwirq);
> 	yy = irq_find_mapping(pcie->msi_domain, data->hwirq);

Do you realize that hwirq is *per domain*, and only makes sense to *one*
irqchip? What you've written here doesn't make any sense.

> 	printk("xx=%d yy=%d\n", xx, yy);
> }
> 
> so data->irq is the virq (30, 34, 35, 36)
> and data->hwirq is the domain hwirq (0, 524288, 524289, 524290)
> 
> Is there a way to map hwirq 524288 to MSI 0, hwirq 524289 to MSI 1, etc?

Why would you need to do such a thing?
- In MSI domain: IRQ34 -> hwirq 524288
- In foo domain: IRQ34 -> hwirq [whatever your driver has allocated]

The data is already there, at your fingertips. Just deal with with your
"foo" irqchip.

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1621963

On 12/04/17 10:50, Mason wrote:
> On 12/04/2017 10:08, Marc Zyngier wrote:
> 
>> On 11/04/17 18:52, Mason wrote:
>>
>>> so data->irq is the virq (30, 34, 35, 36)
>>> and data->hwirq is the domain hwirq (0, 524288, 524289, 524290)
>>>
>>> Is there a way to map hwirq 524288 to MSI 0, hwirq 524289 to MSI 1, etc?
>>
>> Why would you need to do such a thing?
>> - In MSI domain: IRQ34 -> hwirq 524288
>> - In foo domain: IRQ34 -> hwirq [whatever your driver has allocated]
>>
>> The data is already there, at your fingertips. Just deal with with your
>> "foo" irqchip.
> 
> OK, let me take a step back, I may have missed the forest for
> the trees.
> 
> In my original code (copied from the Altera driver) I unmasked
> a given MSI in the tango_irq_domain_alloc() function. You said
> this was the wrong place, as it should be done in the irqchip
> unmask callback. Did I understand correctly, so far?

Yes.

> I have registered custom mask/unmask callbacks for both irq_chips.

And that's *wrong*. I've repeatedly said that you only need to deal with
*your* irqchip. End of story. Nowhere else. Please leave the MSI irqchip
alone, it is very unlikely that you have to provide anything at all to it.

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1626033

On 12/04/2017 11:59, Marc Zyngier wrote:

> And that's *wrong*. I've repeatedly said that you only need to deal with
> *your* irqchip. End of story. Nowhere else. Please leave the MSI irqchip
> alone, it is very unlikely that you have to provide anything at all to it.

Hello Marc,

I am hoping you can help me pinpoint what's missing in my code,
to have it behave the way you describe.

As you have stated, I have *two* struct irq_chip:
one is mine (tango_chip), the other not_mine (msi_chip).

static struct irq_chip tango_chip = {
	.name			= "TANGO IRQ_CHIP",
	.irq_compose_msi_msg	= tango_compose_msi_msg,
	.irq_set_affinity	= tango_set_affinity,
	.irq_ack		= tango_ack,
	.irq_mask		= tango_mask,
	.irq_unmask		= tango_unmask,
};

static struct irq_chip msi_chip = {
	.name = "NOT MY IRQ_CHIP",
	.irq_ack = not_mine_ack,
	.irq_mask = not_mine_mask,
	.irq_unmask = not_mine_unmask,
};


A struct msi_domain_info links to the msi_chip.

static struct msi_domain_info msi_dom_info = {
	.flags	= MSI_FLAG_PCI_MSIX | MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
	.chip	= &msi_chip,
};

irq_dom = irq_domain_create_linear(fwnode, MSI_COUNT, &irq_dom_ops, pcie);
msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);


I believe irq_dom and tango_chip are tied together by:

	irq_domain_set_info(dom, virq, pos, &tango_chip, pcie, handle_edge_irq, NULL, NULL);


My biggest problem is that tango_unmask() is never called.


Here is the log of function calls as they happen on my system:

[    0.361586] TANGO irq_chip=c0d10a00 MSI irq_chip=c0d10aa8
[    0.361595] IRQ_DOM=cf9db300 MSI_DOM=cf9b5080

[    0.363214] find_free_msi: dom=cf9db300 virq=30 hwirq=0 chip=c0d10a00 cdata=cfb0a410
[    0.363314] [<c0316e54>] (tango_irq_domain_alloc) from [<c0163468>] (msi_domain_alloc+0x68/0x128)

[    0.363543] ENTER tango_compose_msi_msg
[    0.363552] TANGO IRQ_CHIP: irq_data=cfaf4180 irq=30 hwirq=0 chip=c0d10a00 dom=cf9db300 pdata=  (null) cdata=cfb0a410
[    0.363609] [<c0316ca4>] (tango_compose_msi_msg) from [<c015f654>] (irq_chip_compose_msi_msg+0x48/0x58)

[    0.363928] ENTER not_mine_unmask
[    0.363943] NOT MY IRQ_CHIP: irq_data=cf940110 irq=30 hwirq=0 chip=c0d10aa8 dom=cf9b5080 pdata=cfaf4180 cdata=  (null)
[    0.364012] [<c0316c48>] (not_mine_unmask) from [<c015edc0>] (irq_enable+0x30/0x44)

[    2.475111] find_free_msi: dom=cf9db300 virq=34 hwirq=1 chip=c0d10a00 cdata=cfb0a410
[    2.516895] [<c0316e54>] (tango_irq_domain_alloc) from [<c0163468>] (msi_domain_alloc+0x68/0x128)

[    2.686593] find_free_msi: dom=cf9db300 virq=35 hwirq=2 chip=c0d10a00 cdata=cfb0a410
[    2.728334] [<c0316e54>] (tango_irq_domain_alloc) from [<c0163468>] (msi_domain_alloc+0x68/0x128)

[    2.897946] find_free_msi: dom=cf9db300 virq=36 hwirq=3 chip=c0d10a00 cdata=cfb0a410
[    2.939680] [<c0316e54>] (tango_irq_domain_alloc) from [<c0163468>] (msi_domain_alloc+0x68/0x128)

[    3.109254] ENTER tango_compose_msi_msg
[    3.114603] TANGO IRQ_CHIP: irq_data=cfb4b500 irq=34 hwirq=1 chip=c0d10a00 dom=cf9db300 pdata=  (null) cdata=cfb0a410
[    3.160621] [<c0316ca4>] (tango_compose_msi_msg) from [<c015f654>] (irq_chip_compose_msi_msg+0x48/0x58)

[    3.340161] ENTER tango_compose_msi_msg
[    3.345507] TANGO IRQ_CHIP: irq_data=cfb4b440 irq=35 hwirq=2 chip=c0d10a00 dom=cf9db300 pdata=  (null) cdata=cfb0a410
[    3.391522] [<c0316ca4>] (tango_compose_msi_msg) from [<c015f654>] (irq_chip_compose_msi_msg+0x48/0x58)

[    3.571148] ENTER tango_compose_msi_msg
[    3.576503] TANGO IRQ_CHIP: irq_data=cfb4b380 irq=36 hwirq=3 chip=c0d10a00 dom=cf9db300 pdata=  (null) cdata=cfb0a410
[    3.631966] [<c015f654>] (irq_chip_compose_msi_msg) from [<c016362c>] (msi_domain_activate+0x18/0x40)

[    3.802121] ENTER not_mine_unmask
[    3.806937] NOT MY IRQ_CHIP: irq_data=cf95c210 irq=34 hwirq=524288 chip=c0d10aa8 dom=cf9b5080 pdata=cfb4b500 cdata=  (null)
[    3.852943] [<c0316c48>] (not_mine_unmask) from [<c015edc0>] (irq_enable+0x30/0x44)

[    3.998596] ENTER not_mine_unmask
[    4.003410] NOT MY IRQ_CHIP: irq_data=cf95c010 irq=35 hwirq=524289 chip=c0d10aa8 dom=cf9b5080 pdata=cfb4b440 cdata=  (null)
[    4.049415] [<c0316c48>] (not_mine_unmask) from [<c015edc0>] (irq_enable+0x30/0x44)

[    4.195055] ENTER not_mine_unmask
[    4.199870] NOT MY IRQ_CHIP: irq_data=cf97cf10 irq=36 hwirq=524290 chip=c0d10aa8 dom=cf9b5080 pdata=cfb4b380 cdata=  (null)
[    4.245874] [<c0316c48>] (not_mine_unmask) from [<c015edc0>] (irq_enable+0x30/0x44)



It's driving me nuts. Do you know what might be wrong?

Regards.

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

#1627188

On 19/04/2017 13:19, Mason wrote:

> My biggest problem is that tango_unmask() is never called.

FTR, the missing incantation was:
Explicitly calling tango_{mask/unmask/ack} from the corresponding msi_{mask/unmask/ack}

Marc, I have one nagging doubt, wrt splitting MSI line selection
and MSI enable.

tango_irq_domain_alloc : finds an available MSI 'j' to allocate
tango_irq_domain_free : release MSI 'j'
tango_unmask : enable MSI 'j'
tango_mask : disable MSI 'j'

Is the following scenario guaranteed to never happen?

tango_irq_domain_alloc // alloc 0
tango_irq_domain_free  // free 0
tango_irq_domain_alloc // alloc 0
tango_unmask // enable 0
tango_unmask // enable 0 = NOP
tango_mask   // disable 0

In this theoretical scenario, we would be left with a non-functional
MSI 0.

Regards.

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

#1627247

On 20/04/17 09:20, Mason wrote:
> On 19/04/2017 13:19, Mason wrote:
> 
>> My biggest problem is that tango_unmask() is never called.
> 
> FTR, the missing incantation was:
> Explicitly calling tango_{mask/unmask/ack} from the corresponding msi_{mask/unmask/ack}

Using irq_chip_mask_parent and co, you mean?

> Marc, I have one nagging doubt, wrt splitting MSI line selection
> and MSI enable.
> 
> tango_irq_domain_alloc : finds an available MSI 'j' to allocate
> tango_irq_domain_free : release MSI 'j'
> tango_unmask : enable MSI 'j'
> tango_mask : disable MSI 'j'
> 
> Is the following scenario guaranteed to never happen?
> 
> tango_irq_domain_alloc // alloc 0
> tango_irq_domain_free  // free 0
> tango_irq_domain_alloc // alloc 0
> tango_unmask // enable 0
> tango_unmask // enable 0 = NOP
> tango_mask   // disable 0
> 
> In this theoretical scenario, we would be left with a non-functional
> MSI 0.

I'm not sure I get the example above, and what the various alloc/free
calls have to do with anything. If you have unbalanced
enable/disable_irq, you loose. Don't do that.

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1621965

On 12/04/2017 10:08, Marc Zyngier wrote:

> On 11/04/17 18:52, Mason wrote:
> 
>> so data->irq is the virq (30, 34, 35, 36)
>> and data->hwirq is the domain hwirq (0, 524288, 524289, 524290)
>>
>> Is there a way to map hwirq 524288 to MSI 0, hwirq 524289 to MSI 1, etc?
> 
> Why would you need to do such a thing?
> - In MSI domain: IRQ34 -> hwirq 524288
> - In foo domain: IRQ34 -> hwirq [whatever your driver has allocated]
> 
> The data is already there, at your fingertips. Just deal with with your
> "foo" irqchip.

OK, let me take a step back, I may have missed the forest for
the trees.

In my original code (copied from the Altera driver) I unmasked
a given MSI in the tango_irq_domain_alloc() function. You said
this was the wrong place, as it should be done in the irqchip
unmask callback. Did I understand correctly, so far?

I have registered custom mask/unmask callbacks for both irq_chips.

The unmask callback that gets called is the one where hwirqs are
524288 and up. But what I need at that point is the "real" MSI
number, because I need to set bit i in some HW register.

Did I somehow mix up domains in one of the init functions?

Regards.



Current debug code posted below, because natural language is often
too ambiguous.

#define MSI_COUNT 32

struct tango_pcie {
	void __iomem *mux;
	void __iomem *msi_status;
	void __iomem *msi_mask;
	phys_addr_t msi_doorbell;
	struct mutex lock; /* lock for updating msi_mask */
	struct irq_domain *irq_domain;
	struct irq_domain *msi_domain;
	int irq;
};

static void tango_msi_isr(struct irq_desc *desc)
{
	unsigned long status;
	unsigned int pos, virq;
	struct irq_chip *chip = irq_desc_get_chip(desc);
	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);

	chained_irq_enter(chip, desc);

	status = readl_relaxed(pcie->msi_status);
	writel_relaxed(status, pcie->msi_status); /* clear IRQs */

	for_each_set_bit(pos, &status, MSI_COUNT) {
		virq = irq_find_mapping(pcie->irq_domain, pos);
		generic_handle_irq(virq);
	}

	chained_irq_exit(chip, desc);
}

#define HOP printk("\nENTER %s\n", __func__); dump_stack()

void foo_ack(struct irq_data *data)	{ HOP; }
void foo_mask(struct irq_data *data)	{ HOP; pci_msi_mask_irq(data); }
void foo_unmask(struct irq_data *data)	{ HOP; pci_msi_unmask_irq(data); }

static struct irq_chip tango_msi_irq_chip = {
	.name = "MSI_foo",
	.irq_ack = foo_ack,
	.irq_mask = foo_mask,
	.irq_unmask = foo_unmask,
};

static struct msi_domain_info msi_domain_info = {
	.flags	= MSI_FLAG_PCI_MSIX | MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
	.chip	= &tango_msi_irq_chip,
};

static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
{
	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);

	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
	msg->data = data->hwirq;
}

static int tango_set_affinity(struct irq_data *irq_data,
		const struct cpumask *mask, bool force)
{
	 return -EINVAL;
}

void bar_ack(struct irq_data *data)	{ HOP; }
void bar_mask(struct irq_data *data)	{ HOP; }
void bar_unmask(struct irq_data *data)	{ HOP; }

static struct irq_chip tango_msi_chip = {
	.name			= "MSI_bar",
	.irq_compose_msi_msg	= tango_compose_msi_msg,
	.irq_set_affinity	= tango_set_affinity,
	.irq_ack = bar_ack,
	.irq_mask = bar_mask,
	.irq_unmask = bar_unmask,
};

static int find_free_msi(struct irq_domain *dom, unsigned int virq)
{
	unsigned int pos;
	unsigned long mask;
	struct tango_pcie *pcie = dom->host_data;

	mask = readl_relaxed(pcie->msi_mask);
	pos = find_first_zero_bit(&mask, MSI_COUNT);
	if (pos >= MSI_COUNT)
		return -ENOSPC;
	writel_relaxed(mask | BIT(pos), pcie->msi_mask);

	irq_domain_set_info(dom, virq, pos, &tango_msi_chip,
			dom->host_data, handle_edge_irq, NULL, NULL);

	return 0;
}

static int tango_irq_domain_alloc(struct irq_domain *dom,
		unsigned int virq, unsigned int nr_irqs, void *args)
{
	int err;
	struct tango_pcie *pcie = dom->host_data;

	mutex_lock(&pcie->lock);
	err = find_free_msi(dom, virq);
	mutex_unlock(&pcie->lock);

	return err;
}

static void tango_irq_domain_free(struct irq_domain *dom,
		unsigned int virq, unsigned int nr_irqs)
{
	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
	struct tango_pcie *pcie = irq_data_get_irq_chip_data(d);
	unsigned int mask, pos = d->hwirq;

	mutex_lock(&pcie->lock);
	mask = readl_relaxed(pcie->msi_mask);
	writel_relaxed(mask & ~BIT(pos), pcie->msi_mask);
	mutex_unlock(&pcie->lock);
}

static const struct irq_domain_ops msi_dom_ops = {
	.alloc	= tango_irq_domain_alloc,
	.free	= tango_irq_domain_free,
};

static int tango_msi_remove(struct platform_device *pdev)
{
	struct tango_pcie *pcie = platform_get_drvdata(pdev);

	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
	irq_domain_remove(pcie->msi_domain);
	irq_domain_remove(pcie->irq_domain);

	return 0;
}

static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
{
	int virq;
	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
	struct irq_domain *msi_dom, *irq_dom;

	mutex_init(&pcie->lock);
	writel_relaxed(0, pcie->msi_mask);

	irq_dom = irq_domain_create_linear(fwnode, MSI_COUNT, &msi_dom_ops, pcie);
	if (!irq_dom) {
		pr_err("Failed to create IRQ domain\n");
		return -ENOMEM;
	}

	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_domain_info, irq_dom);
	if (!msi_dom) {
		pr_err("Failed to create MSI domain\n");
		irq_domain_remove(irq_dom);
		return -ENOMEM;
	}

	virq = platform_get_irq(pdev, 1);
	if (virq <= 0) {
		pr_err("Failed to map IRQ\n");
		irq_domain_remove(msi_dom);
		irq_domain_remove(irq_dom);
		return -ENXIO;
	}

	pcie->irq_domain = irq_dom;
	pcie->msi_domain = msi_dom;
	pcie->irq = virq;
	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);

	return 0;
}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web