Re: [patch 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip

From: Yun Wu (Abel)
Date: Tue Nov 18 2014 - 08:17:33 EST

On 2014/11/18 20:43, Jiang Liu wrote:

> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:02, Thomas Gleixner wrote:
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/12 21:42, Thomas Gleixner wrote:
>>>>> +int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>>>> +{
>>>>> + struct irq_data *pos = NULL;
>>>>> +
>>>>> + for (; data; data = data->parent_data)
>>>>> +#endif
>>>>> + if (data->chip && data->chip->irq_compose_msi_msg)
>>>>> + pos = data;
>>>>> + if (!pos)
>>>>> + return -ENOSYS;
>>>>> +
>>>>> + pos->chip->irq_compose_msi_msg(pos, msg);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>> Adding message composing routine to struct irq_chip is OK to me, and it should
>>>> be because it is interrupt controllers' duty to compose messages (so that they
>>>> can parse the messages correctly without any pre-defined rules that endpoint
>>>> devices absolutely need not to know).
>>>> However a problem comes out when deciding which parameters should be passed to
>>>> this routine. A message can associate with multiple interrupts, which makes me
>>>> think composing messages for each interrupt is not that appropriate. And we
>>>> can take a look at the new routine irq_chip_compose_msi_msg(). It is called by
>>>> msi_domain_activate() which will be called by irq_domain_activate_irq() in
>>>> irq_startup() for each interrupt descriptor, result in composing a message for
>>>> each interrupt, right? (Unless requiring a judge on the parameter @data when
>>>> implementing the irq_compose_msi_msg() callback that only compose message for
>>>> the first entry of that message. But I really don't like that...)
>>> No, that's not correct. You are looking at some random stale version
>>> of this. The current state of affairs is in
>>> git:// irq/irqdomain
>>> See also
>>> In activate we write the message, which is the right point to do so.
>> I checked the current state, it seems to be the same.
>> Yes, the decision of postponing the actual hardware programming to the point
>> where the interrupt actually gets used is right, but here above I was talking
>> another thing.
>> As I mentioned, a message can associate with multiple interrupts. Enabling
>> any of them will call irq_startup(). So if we don't want to compose or write
>> messages repeatedly, we'd better require performing some checks before
>> activating the interrupts.
> Hi Yun,
> Seems you are talking about the case of multiple MSI support.
> Yes, we have special treatment for multiple MSI, which only writes PCI
> MSI registers when starting up the first MSI interrupt.
> void pci_msi_domain_write_msg(struct irq_data *irq_data, struct msi_msg
> *msg)
> {
> struct msi_desc *desc = irq_data->msi_desc;
> /*
> * For MSI-X desc->irq is always equal to irq_data->irq. For
> * MSI only the first interrupt of MULTI MSI passes the test.
> */
> if (desc->irq == irq_data->irq)
> __pci_write_msi_msg(desc, msg);
> }

Yes, I picked the case of multiple MSI support.
The check should also be performed when composing messages. That's why
I don't like its parameters. The @data only indicates one interrupt,
while I prefer doing compose/write in the unit of message descriptor.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at