From: Scott Wood <scottwood@freescale•com>
To: Johannes Thumshirn <johannes.thumshirn@men•de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix•de>,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH 1/2] irqdomain: add support for creating a continous mapping
Date: Mon, 3 Nov 2014 16:18:51 -0600 [thread overview]
Message-ID: <1415053131.23458.287.camel@snotra.buserror.net> (raw)
In-Reply-To: <1415031514-27284-2-git-send-email-johannes.thumshirn@men.de>
On Mon, 2014-11-03 at 17:18 +0100, Johannes Thumshirn wrote:
> A MSI device may have multiple interrupts. That means that the
> interrupts numbers should be continuos so that pdev->irq refers to the
> first interrupt, pdev->irq + 1 to the second and so on.
> This patch adds support for continuous allocation of virqs for a range
> of hwirqs. The function is based on irq_create_mapping() but due to the
> number argument there is very little in common now.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix•de>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@men•de>
> ---
> include/linux/irqdomain.h | 10 ++++--
> kernel/irq/irqdomain.c | 85 ++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 73 insertions(+), 22 deletions(-)
This is changing core kernel code and thus LKML should be CCed, as well
as Ben Herrenschmidt who is the maintainer of kernel/irq/irqdomain.c.
Also please respond to feedback in
http://patchwork.ozlabs.org/patch/322497/
Is it really necessary for the virqs to be contiguous? How is the
availability of multiple MSIs communicated to the driver? Is there an
example of a driver that currently uses multiple MSIs?
> /**
> - * irq_create_mapping() - Map a hardware interrupt into linux irq space
> + * irq_create_mapping_block() - Map multiple hardware interrupts
> * @domain: domain owning this hardware interrupt or NULL for default domain
> * @hwirq: hardware irq number in that domain space
> + * @num: number of interrupts
> + *
> + * Maps a hwirq to a newly allocated virq. If num is greater than 1 then num
> + * hwirqs (hwirq ... hwirq + num - 1) will be mapped and virq will be
> + * continuous.
> + * Returns the first linux virq number.
> *
> - * Only one mapping per hardware interrupt is permitted. Returns a linux
> - * irq number.
> * If the sense/trigger is to be specified, set_irq_type() should be called
> * on the number returned from that call.
> */
> -unsigned int irq_create_mapping(struct irq_domain *domain,
> +unsigned int irq_create_mapping_block(struct irq_domain *domain,
> irq_hw_number_t hwirq)
> {
Where is the num parameter? How does this even build?
> unsigned int hint;
> int virq;
> + int node;
> + int i;
>
> - pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
> + pr_debug("%s(0x%p, 0x%lx, %d)\n", __func__, domain, hwirq, num);
>
> /* Look for default domain if nececssary */
> - if (domain == NULL)
> + if (!domain && num == 1)
> domain = irq_default_domain;
> +
> if (domain == NULL) {
> WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
> return 0;
> @@ -403,35 +437,46 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> pr_debug("-> using domain @%p\n", domain);
>
> /* Check if mapping already exists */
> - virq = irq_find_mapping(domain, hwirq);
> - if (virq) {
> - pr_debug("-> existing mapping on virq %d\n", virq);
> - return virq;
> + for (i = 0; i < num; i++) {
> + virq = irq_find_mapping(domain, hwirq + i);
> + if (virq != NO_IRQ) {
Please don't introduce new uses of NO_IRQ. irq_find_mapping() returns
zero on failure. Some architectures (e.g. ARM) define NO_IRQ as
something other than zero, which will cause this to break.
> + if (i == 0)
> + return irq_check_continuous_mapping(domain,
> + hwirq, num);
> + pr_err("irq: hwirq %ld has no mapping but hwirq %ld "
> + "maps to virq %d. This can't be a block\n",
> + hwirq, hwirq + i, virq);
> + return -EINVAL;
> + }
> }
Explain how you'd get into this error state, and how you'd avoid doing
so.
> + node = of_node_to_nid(domain->of_node);
> +
> /* Allocate a virtual interrupt number */
> hint = hwirq % nr_irqs;
> if (hint == 0)
> hint++;
> - virq = irq_alloc_desc_from(hint, of_node_to_nid(domain->of_node));
> - if (virq <= 0)
> - virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
> + virq = irq_alloc_desc_from(hint, node);
> + if (virq <= 0 && hint != 1)
> + virq = irq_alloc_desc_from(1, node);
Factoring out node seems irrelevant to, and obscures, what you're doing
which is adding a chcek for hint. Why is a hint value of 1 special?
You're also still allocating only one virq, unlike in
http://patchwork.ozlabs.org/patch/322497/
-Scott
next prev parent reply other threads:[~2014-11-03 22:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-03 16:18 [PATCH 0/2] Support multiple MSI interrupts on FSL-MPIC Johannes Thumshirn
2014-11-03 16:18 ` [PATCH 1/2] irqdomain: add support for creating a continous mapping Johannes Thumshirn
2014-11-03 22:18 ` Scott Wood [this message]
2014-11-04 8:35 ` Sebastian Andrzej Siewior
2014-11-05 16:55 ` Johannes Thumshirn
2014-11-03 16:18 ` [PATCH 2/2] powerpc: msi: fsl: add support for multiple MSI interrupts Johannes Thumshirn
2014-11-03 17:51 ` [PATCH 0/2] Support multiple MSI interrupts on FSL-MPIC Scott Wood
-- strict thread matches above, loose matches on Subject: below --
2014-02-20 20:53 Sebastian Andrzej Siewior
2014-02-20 20:53 ` [PATCH 1/2] irqdomain: add support for creating a continous mapping Sebastian Andrzej Siewior
2014-02-20 21:06 ` Scott Wood
2014-02-21 8:04 ` Sebastian Andrzej Siewior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1415053131.23458.287.camel@snotra.buserror.net \
--to=scottwood@freescale$(echo .)com \
--cc=bigeasy@linutronix$(echo .)de \
--cc=johannes.thumshirn@men$(echo .)de \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox