On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:
> Adding a function irq_create_mapping_many() which can associate
> multiple MSIs to a continous irq mapping.
>
> This is needed to enable multiple MSI support for pSeries.
>
> Signed-off-by: Mike Qiu <[email protected]>
> ---
> include/linux/irq.h | 2 +
> include/linux/irqdomain.h | 3 ++
> kernel/irq/irqdomain.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 60ef45b..e00a7ec 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> #define irq_alloc_desc_from(from, node) \
> irq_alloc_descs(-1, from, 1, node)
>
> +#define irq_alloc_desc_n(nevc, node) \
> + irq_alloc_descs(-1, 0, nevc, node)
This has been superseeded by irq_alloc_descs_from(), which is the right
way to do it.
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 0d5b17b..831dded 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -168,6 +168,9 @@ extern int irq_create_strict_mappings(struct irq_domain *domain,
> unsigned int irq_base,
> irq_hw_number_t hwirq_base, int count);
>
> +extern int irq_create_mapping_many(struct irq_domain *domain,
> + irq_hw_number_t hwirq_base, int count);
> +
> static inline int irq_create_identity_mapping(struct irq_domain *host,
> irq_hw_number_t hwirq)
> {
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 96f3a1d..38648e6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
> }
> EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
>
> +/**
> + * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
> + * @domain: domain owning the interrupt range
> + * @hwirq_base: beginning of continuous hardware IRQ range
> + * @count: Number of interrupts to map
For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.
So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.
> + * This routine is used for allocating and mapping a range of hardware
> + * irqs to virtual IRQs where the virtual irq numbers are not at pre-defined
> + * locations.
This comment doesn't make sense to me.
> + *
> + * Greater than 0 is returned upon success, while any failure to establish a
> + * static mapping is treated as an error.
> + */
> +int irq_create_mapping_many(struct irq_domain *domain,
> + irq_hw_number_t hwirq_base, int count)
> +{
> + int ret, irq_base;
> + int virq, i;
> +
> + pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq_base);
I'd like to see this whole function rewritten to reduce the duplication
vs irq_create_mapping(). I don't see any reason why this can't be the
core routine, and irq_create_mapping() becomes a caller of it, passing a
count of 1 ?
> + /* Look for default domain if nececssary */
> + if (!domain)
> + domain = irq_default_domain;
> + if (!domain) {
> + pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
> + , hwirq_base);
> + WARN_ON(1);
> + return 0;
> + }
> + pr_debug("-> using domain @%p\n", domain);
> +
> + /* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
> + if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
> + return irq_domain_legacy_revmap(domain, hwirq_base);
The above doesn't work.
> + /* Check if mapping already exists */
> + for (i = 0; i < count; i++) {
> + virq = irq_find_mapping(domain, hwirq_base+i);
> + if (virq) {
> + pr_debug("existing mapping on virq %d,"
> + " now dispose it first\n", virq);
> + irq_dispose_mapping(virq);
You might have just disposed of someone elses mapping, we shouldn't do
that. It should be an error to the caller.
cheers
于 2013/3/5 10:23, Michael Ellerman 写道:
> On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:
>> Adding a function irq_create_mapping_many() which can associate
>> multiple MSIs to a continous irq mapping.
>>
>> This is needed to enable multiple MSI support for pSeries.
>>
>> Signed-off-by: Mike Qiu <[email protected]>
>> ---
>> include/linux/irq.h | 2 +
>> include/linux/irqdomain.h | 3 ++
>> kernel/irq/irqdomain.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 66 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 60ef45b..e00a7ec 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>> #define irq_alloc_desc_from(from, node) \
>> irq_alloc_descs(-1, from, 1, node)
>>
>> +#define irq_alloc_desc_n(nevc, node) \
>> + irq_alloc_descs(-1, 0, nevc, node)
> This has been superseeded by irq_alloc_descs_from(), which is the right
> way to do it.
Yes, but irq_alloc_descs_from() just for 1 irq, and if I change the api,
maybe a lot places which call this
function will be affact.
>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 0d5b17b..831dded 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -168,6 +168,9 @@ extern int irq_create_strict_mappings(struct irq_domain *domain,
>> unsigned int irq_base,
>> irq_hw_number_t hwirq_base, int count);
>>
>> +extern int irq_create_mapping_many(struct irq_domain *domain,
>> + irq_hw_number_t hwirq_base, int count);
>> +
>> static inline int irq_create_identity_mapping(struct irq_domain *host,
>> irq_hw_number_t hwirq)
>> {
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 96f3a1d..38648e6 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
>> }
>> EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
>>
>> +/**
>> + * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
>> + * @domain: domain owning the interrupt range
>> + * @hwirq_base: beginning of continuous hardware IRQ range
>> + * @count: Number of interrupts to map
> For multiple-MSI the allocated interrupt numbers must be a power-of-2,
> and must be naturally aligned. I don't /think/ that's a requirement for
> the virtual numbers, but it's probably best that we do it anyway.
>
> So this API needs to specify that it will give you back a power-of-2
> block that is naturally aligned - otherwise you can't use it for MSI.
rtas_call will return the numbers of hardware interrupt, and it should
be power-of-2,
as this I think do not need to specify
>> + * This routine is used for allocating and mapping a range of hardware
>> + * irqs to virtual IRQs where the virtual irq numbers are not at pre-defined
>> + * locations.
> This comment doesn't make sense to me.
>
>> + *
>> + * Greater than 0 is returned upon success, while any failure to establish a
>> + * static mapping is treated as an error.
>> + */
>> +int irq_create_mapping_many(struct irq_domain *domain,
>> + irq_hw_number_t hwirq_base, int count)
>> +{
>> + int ret, irq_base;
>> + int virq, i;
>> +
>> + pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq_base);
>
> I'd like to see this whole function rewritten to reduce the duplication
> vs irq_create_mapping(). I don't see any reason why this can't be the
> core routine, and irq_create_mapping() becomes a caller of it, passing a
> count of 1 ?
It's good suggestion.
>> + /* Look for default domain if nececssary */
>> + if (!domain)
>> + domain = irq_default_domain;
>> + if (!domain) {
>> + pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
>> + , hwirq_base);
>> + WARN_ON(1);
>> + return 0;
>> + }
>> + pr_debug("-> using domain @%p\n", domain);
>> +
>> + /* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
>> + if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
>> + return irq_domain_legacy_revmap(domain, hwirq_base);
> The above doesn't work.
Why it doesn't work ?
>> + /* Check if mapping already exists */
>> + for (i = 0; i < count; i++) {
>> + virq = irq_find_mapping(domain, hwirq_base+i);
>> + if (virq) {
>> + pr_debug("existing mapping on virq %d,"
>> + " now dispose it first\n", virq);
>> + irq_dispose_mapping(virq);
> You might have just disposed of someone elses mapping, we shouldn't do
> that. It should be an error to the caller.
It's a good question. If the interrupt used for someone elses, why I can
apply it from the system?
So it may someone else forget to dispose mapping, and it never be used
for others as I have got
the interrupt I think.
> cheers
>
On Tue, Mar 05, 2013 at 03:19:57PM +0800, Mike Qiu wrote:
> 于 2013/3/5 10:23, Michael Ellerman 写道:
> >On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:
> >>Adding a function irq_create_mapping_many() which can associate
> >>multiple MSIs to a continous irq mapping.
> >>
> >>This is needed to enable multiple MSI support for pSeries.
> >>
> >>Signed-off-by: Mike Qiu <[email protected]>
> >>---
> >> include/linux/irq.h | 2 +
> >> include/linux/irqdomain.h | 3 ++
> >> kernel/irq/irqdomain.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 66 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/include/linux/irq.h b/include/linux/irq.h
> >>index 60ef45b..e00a7ec 100644
> >>--- a/include/linux/irq.h
> >>+++ b/include/linux/irq.h
> >>@@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> >> #define irq_alloc_desc_from(from, node) \
> >> irq_alloc_descs(-1, from, 1, node)
> >>+#define irq_alloc_desc_n(nevc, node) \
> >>+ irq_alloc_descs(-1, 0, nevc, node)
> >This has been superseeded by irq_alloc_descs_from(), which is the right
> >way to do it.
> Yes, but irq_alloc_descs_from() just for 1 irq
No it's not, look again.
#define irq_alloc_descs_from(from, cnt, node) \
irq_alloc_descs(-1, from, cnt, node)
> >>diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> >>index 96f3a1d..38648e6 100644
> >>--- a/kernel/irq/irqdomain.c
> >>+++ b/kernel/irq/irqdomain.c
> >>@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
> >> }
> >> EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
> >>+/**
> >>+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
> >>+ * @domain: domain owning the interrupt range
> >>+ * @hwirq_base: beginning of continuous hardware IRQ range
> >>+ * @count: Number of interrupts to map
> >For multiple-MSI the allocated interrupt numbers must be a power-of-2,
> >and must be naturally aligned. I don't /think/ that's a requirement for
> >the virtual numbers, but it's probably best that we do it anyway.
> >
> >So this API needs to specify that it will give you back a power-of-2
> >block that is naturally aligned - otherwise you can't use it for MSI.
> rtas_call will return the numbers of hardware interrupt, and it
> should be power-of-2, as this I think do not need to specify
You're confusing hardware interrupt numbers and virtual interrupt
numbers. My comment is about irq_create_mapping_many(), which returns
virtual interrupt numbers.
As I said I don't think there is a requirement that the virtual
interrupt numbers are also a power-of-2 naturally aligned block, but we
should allocate them as one anyway, to avoid any issues in future.
And so this API, which returns virtual interrupt numbers, must satisfy
that specification.
> >>+ /* Look for default domain if nececssary */
> >>+ if (!domain)
> >>+ domain = irq_default_domain;
> >>+ if (!domain) {
> >>+ pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
> >>+ , hwirq_base);
> >>+ WARN_ON(1);
> >>+ return 0;
> >>+ }
> >>+ pr_debug("-> using domain @%p\n", domain);
> >>+
> >>+ /* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
> >>+ if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
> >>+ return irq_domain_legacy_revmap(domain, hwirq_base);
> >The above doesn't work.
> Why it doesn't work ?
Because irq_domain_legacy_revmap() only allocates a single interrupt
number.
> >>+ /* Check if mapping already exists */
> >>+ for (i = 0; i < count; i++) {
> >>+ virq = irq_find_mapping(domain, hwirq_base+i);
> >>+ if (virq) {
> >>+ pr_debug("existing mapping on virq %d,"
> >>+ " now dispose it first\n", virq);
> >>+ irq_dispose_mapping(virq);
> >You might have just disposed of someone elses mapping, we shouldn't do
> >that. It should be an error to the caller.
> It's a good question. If the interrupt used for someone elses, why I
> can apply it from the system?
I agree, that would be a bug. But disposing of someone elses mapping is
not OK.
> So it may someone else forget to dispose mapping, and it never be
> used for others as I have got the interrupt I think.
Perhaps, but that is a bug that needs to be fixed in the code that
forgets to dispose of the mapping.
cheers