Messages in this thread | | | Date | Tue, 18 Nov 2014 21:25:35 +0800 | From | Jiang Liu <> | Subject | Re: [patch 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip |
| |
On 2014/11/18 21:16, Yun Wu (Abel) wrote: > 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; >>>>>> + >>>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY >>>>>> + 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://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/irqdomain >>>> >>>> See also https://lkml.org/lkml/2014/11/17/764 >>>> >>>> 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. Hi Yun, The common abstraction is that every message interrupt could be controlled independently, so have compose_msi_msg()/write_msi_msg() per interrupt. MSI is abstracted as an special message signaled interrupt with hardware limitation where multiple interrupts sharing the same hardware registers. So we filter in pci_msi_domain_write_msg(). On the other handle, the generic MSI framework caches msi_msg in msi_desc, so we don't filter compose_msi_msg(). Regards! Gerry
| |