From: Jiang Liu <[email protected]>
Add callback irq_compose_msi_msg to struct irq_chip, which will be used
to support stacked irqchip.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Yingjoe Chen <[email protected]>
Cc: Yijing Wang <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
include/linux/irq.h | 5 +++++
kernel/irq/chip.c | 17 +++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0adcbbbf2e87..536b7fc6c8f4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -29,6 +29,7 @@ struct seq_file;
struct module;
struct irq_desc;
struct irq_data;
+struct msi_msg;
typedef void (*irq_flow_handler_t)(unsigned int irq,
struct irq_desc *desc);
typedef void (*irq_preflow_handler_t)(struct irq_data *data);
@@ -320,6 +321,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
* any other callback related to this irq
* @irq_release_resources: optional to release resources acquired with
* irq_request_resources
+ * @irq_compose_msi_msg: optional to compose message content for MSI
* @flags: chip specific flags
*/
struct irq_chip {
@@ -356,6 +358,8 @@ struct irq_chip {
int (*irq_request_resources)(struct irq_data *data);
void (*irq_release_resources)(struct irq_data *data);
+ void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
+
unsigned long flags;
};
@@ -443,6 +447,7 @@ extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_nested_irq(unsigned int irq);
+extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void irq_chip_ack_parent(struct irq_data *data);
extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 12f3e72449eb..8f362db17a8a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -867,3 +867,20 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
return -ENOSYS;
}
#endif
+
+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;
+}
--
1.7.10.4
On 2014/11/12 21:42, Thomas Gleixner wrote:
> From: Jiang Liu <[email protected]>
>
> Add callback irq_compose_msi_msg to struct irq_chip, which will be used
> to support stacked irqchip.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Yijing Wang <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> include/linux/irq.h | 5 +++++
> kernel/irq/chip.c | 17 +++++++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 0adcbbbf2e87..536b7fc6c8f4 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -29,6 +29,7 @@ struct seq_file;
> struct module;
> struct irq_desc;
> struct irq_data;
> +struct msi_msg;
> typedef void (*irq_flow_handler_t)(unsigned int irq,
> struct irq_desc *desc);
> typedef void (*irq_preflow_handler_t)(struct irq_data *data);
> @@ -320,6 +321,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> * any other callback related to this irq
> * @irq_release_resources: optional to release resources acquired with
> * irq_request_resources
> + * @irq_compose_msi_msg: optional to compose message content for MSI
> * @flags: chip specific flags
> */
> struct irq_chip {
> @@ -356,6 +358,8 @@ struct irq_chip {
> int (*irq_request_resources)(struct irq_data *data);
> void (*irq_release_resources)(struct irq_data *data);
>
> + void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
> +
> unsigned long flags;
> };
>
> @@ -443,6 +447,7 @@ extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
> extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
> extern void handle_nested_irq(unsigned int irq);
>
> +extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg);
> #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> extern void irq_chip_ack_parent(struct irq_data *data);
> extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 12f3e72449eb..8f362db17a8a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -867,3 +867,20 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
> return -ENOSYS;
> }
> #endif
> +
> +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...)
Regards,
Abel
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.
Thanks,
tglx
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.
Thanks,
Abel
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);
}
Regards!
Gerry
>
> Thanks,
> Abel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
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.
Thanks,
Abel
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
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.
Thanks,
Abel
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.
>
> Thanks,
> Abel
>
>
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
On 2014/11/18 22:03, Yun Wu (Abel) wrote:
> 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?
Basically, yes:)