| Message ID | 1379320326-13241-3-git-send-email-treding@nvidia.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Sep 16, 2013 at 10:31 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > This is a version of irq_create_mapping() that propagates the precise > error code instead of returning 0 for all errors. It will be used in > subsequent patches to allow further propagation of error codes. > > To avoid code duplication, implement irq_create_mapping() as a wrapper > around the new __irq_create_mapping(). > > Signed-off-by: Thierry Reding <treding@nvidia.com> Surprise! I don't like this. I think it is better to first go over the call sites and make them all handle negative return numbers rather than pushing the obscure __interface. I know from patch 0 that you think it's too much to change these 127 call sites but I don't think so, and I'm happy to merge one big patch changing all the 20 users in drivers/gpio. Likewise with the 11 consumers in drivers/pinctrl. It's just a a few archs+subsystems and it's just plain work. So do that first. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote: > On Mon, Sep 16, 2013 at 10:31 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > > This is a version of irq_create_mapping() that propagates the precise > > error code instead of returning 0 for all errors. It will be used in > > subsequent patches to allow further propagation of error codes. > > > > To avoid code duplication, implement irq_create_mapping() as a wrapper > > around the new __irq_create_mapping(). > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > Surprise! I don't like this. > > I think it is better to first go over the call sites and make them > all handle negative return numbers rather than pushing the > obscure __interface. > > I know from patch 0 that you think it's too much to change these > 127 call sites but I don't think so, and I'm happy to merge one > big patch changing all the 20 users in drivers/gpio. > > Likewise with the 11 consumers in drivers/pinctrl. > > It's just a a few archs+subsystems and it's just plain work. Well, the problem is that the current patch changes the signature of the function as well, therefore the call sites will have to be updated all at once in a single patch to avoid build breakage. And that's excluding any potential fallout from new callsites added between the creation of the patch and its application. Another alternative could be to change the signature in a way that does not break compatibility. For instance I think it could work out if we change this function to return int instead of unsigned int but keep the same semantics to begin with (return 0 on failure). Then update all call sites to handle potential negative errors and after that return negative error codes. That still wouldn't catch any callers introduced between the patch creation and application. Thierry
On Mon, Sep 23, 2013 at 10:29 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote: >> I think it is better to first go over the call sites and make them >> all handle negative return numbers rather than pushing the >> obscure __interface. (...) > > Well, the problem is that the current patch changes the signature of the > function as well, therefore the call sites will have to be updated all > at once in a single patch to avoid build breakage. Hm yeah OK I see the problem, but can we atleast avoid the __thing? Like calling the new function irq_create_mapping_strict() or whatever. > Another alternative could be to change the signature in a way that does > not break compatibility. For instance I think it could work out if we > change this function to return int instead of unsigned int but keep the > same semantics to begin with (return 0 on failure). Then update all call > sites to handle potential negative errors and after that return negative > error codes. Hm that sounds like an attractive solution to me actually. > That still wouldn't catch any callers introduced between > the patch creation and application. Such things happen all the time, just have to be attentive in what goes into linux-next... Another minor thing: +static int __irq_create_mapping(struct irq_domain *domain, + irq_hw_number_t hwirq, unsigned int *virqp) Unless you can make a very good case for why there should be a "v" in the beginning of virqp, then remove it and call it "irqp" simply. All Linux IRQs are virtual and we're already clearly separating out those that are not by calling them "hwirq". Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 24, 2013 at 02:20:44PM +0200, Linus Walleij wrote: > On Mon, Sep 23, 2013 at 10:29 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Mon, Sep 23, 2013 at 09:14:30PM +0200, Linus Walleij wrote: > > >> I think it is better to first go over the call sites and make them > >> all handle negative return numbers rather than pushing the > >> obscure __interface. > (...) > > > > Well, the problem is that the current patch changes the signature of the > > function as well, therefore the call sites will have to be updated all > > at once in a single patch to avoid build breakage. > > Hm yeah OK I see the problem, but can we atleast avoid the > __thing? Like calling the new function irq_create_mapping_strict() > or whatever. _strict sort of implies that it does something more than the non-strict irq_create_mapping() while it really doesn't. Perhaps the alternative proposed below would indeed be a better solution. > > Another alternative could be to change the signature in a way that does > > not break compatibility. For instance I think it could work out if we > > change this function to return int instead of unsigned int but keep the > > same semantics to begin with (return 0 on failure). Then update all call > > sites to handle potential negative errors and after that return negative > > error codes. > > Hm that sounds like an attractive solution to me actually. The only thing we'd loose is the additional bit, but given that most (if not all) platforms that use DT are 32-bit (do we actually support any platforms that don't have 32-bit integers?) that should not matter at all. We're not very likely to get anywhere near that number of interrupts in the system. > > That still wouldn't catch any callers introduced between > > the patch creation and application. > > Such things happen all the time, just have to be attentive in > what goes into linux-next... Given that linux-next might not be with us for much longer before the 3.12 release, I'm thinking of deferring the series until then. Or at least trying to get it merged. Otherwise we'll probably have to deal with a lot of fall out during the merge window. In any case, it'd be nice to get some feedback on the general idea of the patch series from other people involved. I'd hate to do all the conversions just to have it NAKed at the last minute. > Another minor thing: > > +static int __irq_create_mapping(struct irq_domain *domain, > + irq_hw_number_t hwirq, unsigned int *virqp) > > Unless you can make a very good case for why there should > be a "v" in the beginning of virqp, then remove it and call it > "irqp" simply. > > All Linux IRQs are virtual and we're already clearly separating > out those that are not by calling them "hwirq". Yeah, I was just trying to adapt to what was already there. But with the alternative proposal that'll go away anyway. Thierry
On Tue, Sep 24, 2013 at 8:28 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > Given that linux-next might not be with us for much longer before the > 3.12 release, I'm thinking of deferring the series until then. Or at > least trying to get it merged. Otherwise we'll probably have to deal > with a lot of fall out during the merge window. Hm, how unfortunate. Typically this is the kind of topic branch that should go in separately to linux-next. > In any case, it'd be nice to get some feedback on the general idea of > the patch series from other people involved. I'd hate to do all the > conversions just to have it NAKed at the last minute. With the direction we've discussed here atleast I won't be doing any NACKing if it's of any help... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 706724e..d2a3b01 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -374,30 +374,21 @@ unsigned int irq_create_direct_mapping(struct irq_domain *domain) } EXPORT_SYMBOL_GPL(irq_create_direct_mapping); -/** - * irq_create_mapping() - Map a hardware interrupt into linux irq space - * @domain: domain owning this hardware interrupt or NULL for default domain - * @hwirq: hardware irq number in that domain space - * - * 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, - irq_hw_number_t hwirq) +static int __irq_create_mapping(struct irq_domain *domain, + irq_hw_number_t hwirq, unsigned int *virqp) { - unsigned int hint; - int virq; + unsigned int hint, virq; + int ret; - pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq); + pr_debug("__irq_create_mapping(0x%p, 0x%lx, %p)\n", domain, hwirq, + virqp); /* Look for default domain if nececssary */ if (domain == NULL) domain = irq_default_domain; if (domain == NULL) { WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq); - return 0; + return -ENODEV; } pr_debug("-> using domain @%p\n", domain); @@ -405,7 +396,11 @@ unsigned int irq_create_mapping(struct irq_domain *domain, virq = irq_find_mapping(domain, hwirq); if (virq) { pr_debug("-> existing mapping on virq %d\n", virq); - return virq; + + if (virqp) + *virqp = virq; + + return 0; } /* Allocate a virtual interrupt number */ @@ -417,17 +412,41 @@ unsigned int irq_create_mapping(struct irq_domain *domain, virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node)); if (virq <= 0) { pr_debug("-> virq allocation failed\n"); - return 0; + return virq ? : -ENOSPC; } - if (irq_domain_associate(domain, virq, hwirq)) { + ret = irq_domain_associate(domain, virq, hwirq); + if (ret) { irq_free_desc(virq); - return 0; + return ret; } pr_debug("irq %lu on domain %s mapped to virtual irq %u\n", hwirq, of_node_full_name(domain->of_node), virq); + if (virqp) + *virqp = virq; + + return 0; +} +/** + * irq_create_mapping() - Map a hardware interrupt into linux irq space + * @domain: domain owning this hardware interrupt or NULL for default domain + * @hwirq: hardware irq number in that domain space + * + * 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, + irq_hw_number_t hwirq) +{ + unsigned int virq; + + if (__irq_create_mapping(domain, hwirq, &virq)) + return 0; + return virq; } EXPORT_SYMBOL_GPL(irq_create_mapping);
This is a version of irq_create_mapping() that propagates the precise error code instead of returning 0 for all errors. It will be used in subsequent patches to allow further propagation of error codes. To avoid code duplication, implement irq_create_mapping() as a wrapper around the new __irq_create_mapping(). Signed-off-by: Thierry Reding <treding@nvidia.com> --- kernel/irq/irqdomain.c | 59 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 20 deletions(-)