Messages in this thread | | | Date | Tue, 18 Nov 2014 22:03:40 +0800 | From | "Yun Wu (Abel)" <> | Subject | Re: [patch 04/16] genirq: Introduce irq_chip.irq_compose_msi_msg() to support stacked irqchip |
| |
On 2014/11/18 21:55, Jiang Liu wrote:
> On 2014/11/18 21:48, Yun Wu (Abel) wrote: >> On 2014/11/18 21:25, Jiang Liu wrote: >> >>> 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(). >>> >> >> It's true that every message interrupt could be controlled independently, >> I mean, by enable/disable/mask/unmask. But the message data & address are >> shared among the interrupts of that message. >> Despite the detailed hardware implementation, MSI and MSI-X are the same >> thing in software view, that is a message related with several consecutive >> interrupts. And the core MSI infrastructure you want to build should not >> be based on any hardware assumptions. > That's the key point. We abstract MSI as using a message to control an > interrupt source instead of controlling several consecutive interrupts. > PCI MSI is just a special case which controls a group of consecutive > interrupts all together due to hardware limitation. >
Oh, I see. We abstract it in different ways... And sounds like you treat multiple MSI as a broken implementation?
Abel
| |